Bug 83284

Created attachment 135835 [details] patch

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Attachment 135835 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Source/WebKit/chromium/src/BatteryClientChromium.h:48: The parameter name "status" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/src/BatteryClientChromium.h:62: One space before end of line comments [whitespace/comments] [5] Source/WebKit/chromium/src/BatteryClientChromium.h:64: One space before end of line comments [whitespace/comments] [5] Source/WebKit/chromium/src/BatteryClientChromium.h:65: One space before end of line comments [whitespace/comments] [5] Source/WebKit/chromium/src/BatteryClientChromium.cpp:37: Alphabetical sorting problem. [build/include_order] [4] Source/WebKit/chromium/src/BatteryClientChromium.cpp:96: One space before end of line comments [whitespace/comments] [5] Source/WebKit/chromium/src/BatteryClientChromium.cpp:98: One space before end of line comments [whitespace/comments] [5] Source/WebKit/chromium/public/WebBatteryStatus.h:41: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebKit/chromium/public/WebBatteryStatus.h:62: One space before end of line comments [whitespace/comments] [5] Source/WebKit/chromium/public/WebBatteryStatus.h:64: One space before end of line comments [whitespace/comments] [5] Source/WebCore/ChangeLog:10: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 11 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 135836 [details] Patch

Comment on attachment 135836 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135836&action=review and more tests? > Source/WebKit/chromium/public/WebBatteryStatus.h:34 > +public: is this a serialized class? you would propose pushing WBS instances from RenderView? How/when does that get pushed? > Source/WebKit/chromium/public/WebBatteryStatus.h:39 > + , level(0.0) { } put the braces on a lower line > Source/WebKit/chromium/public/WebBatteryStatus.h:41 > + bool operator==(const WebBatteryStatus& status) const i would prefer a method instead of an operator. Possibly a static. > Source/WebKit/chromium/src/BatteryClientChromium.h:57 > + WebViewClient* m_client; refptrs or ownptrs here?

Comment on attachment 135836 [details] Patch Attachment 135836 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12264389 New failing tests: fast/dom/navigator-detached-no-crash.html

Created attachment 135851 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 135836 [details] Patch Attachment 135836 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12339425 New failing tests: fast/dom/navigator-detached-no-crash.html

Created attachment 135868 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 135836 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135836&action=review >> Source/WebKit/chromium/public/WebBatteryStatus.h:34 >> +public: > > is this a serialized class? > > you would propose pushing WBS instances from RenderView? How/when does that get pushed? Indeed. This is like WebInputEvent, where it gets created in the browser process and sent to the renderer over IPC. This will be sent whenever the battery-status changes (so, every time the battery-icon in the status area in chromeos gets updated, it will also get this notification). It may make sense to throttle/fine-tune exactly when this should be sent (this will in the chromium browser process, of course). >> Source/WebKit/chromium/public/WebBatteryStatus.h:39 >> + , level(0.0) { } > > put the braces on a lower line The style seems to be that empty bodies go in the same line as the last param. >> Source/WebKit/chromium/public/WebBatteryStatus.h:41 >> + bool operator==(const WebBatteryStatus& status) const > > i would prefer a method instead of an operator. Possibly a static. The use of operator-overloading for == seems also to be the norm (at least in chromium/public/) :) >> Source/WebKit/chromium/src/BatteryClientChromium.h:57 >> + WebViewClient* m_client; > > refptrs or ownptrs here? I am not sure about this one. I don't believe this controller owns the client, and I am unsure if it needs to hold a refptr on the client, since the WebViewClient owns the BatteryClientChromium.

Comment on attachment 135836 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135836&action=review > Source/WebKit/chromium/public/WebBatteryStatus.h:60 > + bool charging; > + double chargingTime; > + double dischargingTime; > + double level; I would have expected this class to be a wrapper around WebCore::BatteryStatus, like WebNode is a wrapper around WebCore::Node. > Source/WebKit/chromium/public/WebViewClient.h:328 > + // Battery Status ------------------------------------------------------ > + > + // Notifies the client to start or stop sending battery status updates. > + virtual void startUpdatingBatteryStatus() { } > + virtual void stopUpdatingBatteryStatus() { } Should these functions be on a WebBatteryStatusClient? (Compare with how Geolocation works.) The battery status isn't really a concern of the WebView. The WebView is just the context object. > Source/WebKit/chromium/public/WebWidget.h:212 > + virtual void handleBatteryStatusUpdate(const WebBatteryStatus&) { } Why on the WebWidget? I would have expected this function to be on the WebView. Also, why not call it updateBatteryStatus? "Update" is a much stronger verb than "handle". > Source/WebKit/chromium/src/BatteryClientChromium.cpp:52 > +void BatteryClientChromium::setBatteryStatus(const WebBatteryStatus& webstatus) webstatus -> webStatus ? > Source/WebKit/chromium/src/BatteryClientChromium.cpp:58 > + RefPtr<WebCore::BatteryStatus> status = WebCore::BatteryStatus::create(webstatus.charging, webstatus.chargingTime, webstatus.dischargingTime, webstatus.level); WebCore::BatteryStatus <-- If we're importing the whole WebCore namespace, the WebCore:: prefix shouldn't be needed. I think we're supposed to import WebCore symbols individually though. > Source/WebKit/chromium/src/BatteryClientChromium.cpp:64 > + if (m_batteryStatus.charging != webstatus.charging) > + m_controller->didChangeBatteryStatus(eventNames().chargingchangeEvent, status); > + else if (webstatus.charging && m_batteryStatus.chargingTime != webstatus.chargingTime) > + m_controller->didChangeBatteryStatus(eventNames().chargingtimechangeEvent, status); > + else if (!webstatus.charging && m_batteryStatus.dischargingTime != webstatus.dischargingTime) > + m_controller->didChangeBatteryStatus(eventNames().dischargingtimechangeEvent, status); This work seems like it should be done by WebCore. Does the BatteryStatusController not have an updateBatteryStatus method? >>> Source/WebKit/chromium/src/BatteryClientChromium.h:57 >>> + WebViewClient* m_client; >> >> refptrs or ownptrs here? > > I am not sure about this one. I don't believe this controller owns the client, and I am unsure if it needs to hold a refptr on the client, since the WebViewClient owns the BatteryClientChromium. Normally, BatteryClientChromium would hold a pointer to the WebBatteryClient directly rather than to the whole WebViewClient. > Source/WebKit/chromium/src/BatteryClientChromium.h:65 > +#endif // BatteryClientChromium_h We usually have a blank line above here. > Source/WebKit/chromium/src/WebViewImpl.cpp:1365 > +void WebViewImpl::handleBatteryStatusUpdate(const WebBatteryStatus& status) > +{ > + m_batteryClient->setBatteryStatus(status); > +} Why does the name change here? It seems like we can call both functions updateBatteryStatus.

Created attachment 136056 [details] Patch

Comment on attachment 135836 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135836&action=review >> Source/WebKit/chromium/public/WebBatteryStatus.h:60 >> + double level; > > I would have expected this class to be a wrapper around WebCore::BatteryStatus, like WebNode is a wrapper around WebCore::Node. In this case, the battery-status is being created in the browser-side, and being sent, across IPC, and eventually to webkit. So it feels like this should be more like WebInputEvent than WebNode. Does that make sense? >> Source/WebKit/chromium/public/WebViewClient.h:328 >> + virtual void stopUpdatingBatteryStatus() { } > > Should these functions be on a WebBatteryStatusClient? (Compare with how Geolocation works.) The battery status isn't really a concern of the WebView. The WebView is just the context object. Sounds like a good plan. Done. >> Source/WebKit/chromium/public/WebWidget.h:212 >> + virtual void handleBatteryStatusUpdate(const WebBatteryStatus&) { } > > Why on the WebWidget? I would have expected this function to be on the WebView. > > Also, why not call it updateBatteryStatus? "Update" is a much stronger verb than "handle". I agree. Renamed as suggested, and moved to WebView. Thanks! >> Source/WebKit/chromium/src/BatteryClientChromium.cpp:52 >> +void BatteryClientChromium::setBatteryStatus(const WebBatteryStatus& webstatus) > > webstatus -> webStatus ? Done (renamed to batteryStatus instead since that probably makes more sense) >> Source/WebKit/chromium/src/BatteryClientChromium.cpp:58 >> + RefPtr<WebCore::BatteryStatus> status = WebCore::BatteryStatus::create(webstatus.charging, webstatus.chargingTime, webstatus.dischargingTime, webstatus.level); > > WebCore::BatteryStatus <-- If we're importing the whole WebCore namespace, the WebCore:: prefix shouldn't be needed. I think we're supposed to import WebCore symbols individually though. Removed 'using namespace WebCore;' >> Source/WebKit/chromium/src/BatteryClientChromium.cpp:64 >> + m_controller->didChangeBatteryStatus(eventNames().dischargingtimechangeEvent, status); > > This work seems like it should be done by WebCore. Does the BatteryStatusController not have an updateBatteryStatus method? I agree that this happening in WebCore makes more sense. The BatteryStatusController currently only has didChangeBatteryStatus, and no updateBatteryStatus. I was hesitant to make that change. Should I go ahead and move this in BSC? >>>> Source/WebKit/chromium/src/BatteryClientChromium.h:57 >>>> + WebViewClient* m_client; >>> >>> refptrs or ownptrs here? >> >> I am not sure about this one. I don't believe this controller owns the client, and I am unsure if it needs to hold a refptr on the client, since the WebViewClient owns the BatteryClientChromium. > > Normally, BatteryClientChromium would hold a pointer to the WebBatteryClient directly rather than to the whole WebViewClient. Done. >> Source/WebKit/chromium/src/BatteryClientChromium.h:65 >> +#endif // BatteryClientChromium_h > > We usually have a blank line above here. Done >> Source/WebKit/chromium/src/WebViewImpl.cpp:1365 >> +} > > Why does the name change here? It seems like we can call both functions updateBatteryStatus. Done.

> In this case, the battery-status is being created in the browser-side, and being sent, across IPC, and eventually to webkit. So it feels like this should be more like WebInputEvent than WebNode. Does that make sense? Yeah, that makes sense. There isn't a WebCore object that we need to wrap yet. > >> Source/WebKit/chromium/src/BatteryClientChromium.cpp:64 > >> + m_controller->didChangeBatteryStatus(eventNames().dischargingtimechangeEvent, status); > > > > This work seems like it should be done by WebCore. Does the BatteryStatusController not have an updateBatteryStatus method? > > I agree that this happening in WebCore makes more sense. The BatteryStatusController currently only has didChangeBatteryStatus, and no updateBatteryStatus. I was hesitant to make that change. Should I go ahead and move this in BSC? You shouldn't be hesitant to change WebCore. I'd move the code to BSC. That seems better than each port copy/pasting the same code.

Comment on attachment 136056 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136056&action=review This looks great. Let's do one more round of polish and then I think we're set to land this patch. Thanks! > Source/WebKit/chromium/public/WebBatteryStatus.h:39 > + , level(0.0) { } Technically { and } should each be on their own line. > Source/WebKit/chromium/public/WebWidget.h:46 > +class WebBatteryStatus; This probably isn't needed anymore, right? > Source/WebKit/chromium/src/BatteryClientChromium.cpp:44 > +BatteryClientChromium::BatteryClientChromium(WebBatteryStatusClient* client) BatteryClientChromium -> I might have called this class BatteryClientImpl. It's in Chromium's WebKit layer, so it's a pretty safe bet that it's for Chromium. :) > Source/WebKit/chromium/src/BatteryClientChromium.cpp:65 > + if (m_batteryStatus.charging != batteryStatus.charging) > + m_controller->didChangeBatteryStatus(WebCore::eventNames().chargingchangeEvent, status); > + else if (batteryStatus.charging && m_batteryStatus.chargingTime != batteryStatus.chargingTime) > + m_controller->didChangeBatteryStatus(WebCore::eventNames().chargingtimechangeEvent, status); > + else if (!batteryStatus.charging && m_batteryStatus.dischargingTime != batteryStatus.dischargingTime) > + m_controller->didChangeBatteryStatus(WebCore::eventNames().dischargingtimechangeEvent, status); > + > + if (m_batteryStatus.level != batteryStatus.level) > + m_controller->didChangeBatteryStatus(WebCore::eventNames().levelchangeEvent, status); As discussed, we should move this into WebCore. > Source/WebKit/chromium/src/BatteryClientChromium.h:59 > + WebBatteryStatus m_batteryStatus; This member should probably move into WebCore::BatteryController (and transform into the WebCore version of the class.

Created attachment 136101 [details] Patch

Comment on attachment 136056 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136056&action=review >> Source/WebKit/chromium/public/WebBatteryStatus.h:39 >> + , level(0.0) { } > > Technically { and } should each be on their own line. Done. >> Source/WebKit/chromium/public/WebWidget.h:46 >> +class WebBatteryStatus; > > This probably isn't needed anymore, right? Indeed. Removed. >> Source/WebKit/chromium/src/BatteryClientChromium.cpp:44 >> +BatteryClientChromium::BatteryClientChromium(WebBatteryStatusClient* client) > > BatteryClientChromium -> I might have called this class BatteryClientImpl. It's in Chromium's WebKit layer, so it's a pretty safe bet that it's for Chromium. :) Done. >> Source/WebKit/chromium/src/BatteryClientChromium.cpp:65 >> + m_controller->didChangeBatteryStatus(WebCore::eventNames().levelchangeEvent, status); > > As discussed, we should move this into WebCore. Done. >> Source/WebKit/chromium/src/BatteryClientChromium.h:59 >> + WebBatteryStatus m_batteryStatus; > > This member should probably move into WebCore::BatteryController (and transform into the WebCore version of the class. Done.

Comment on attachment 136101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136101&action=review > Source/WebKit/chromium/src/BatteryClientImpl.cpp:54 > + m_controller->updateBatteryStatus(status); status.release() > Source/WebKit/chromium/src/BatteryClientImpl.cpp:77 > +void BatteryClientImpl::batteryControllerDestroyed() > +{ > +} Shouldn't we set m_controller = 0 ?

Created attachment 136104 [details] Patch

Comment on attachment 136101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136101&action=review >> Source/WebKit/chromium/src/BatteryClientImpl.cpp:54 >> + m_controller->updateBatteryStatus(status); > > status.release() Done. >> Source/WebKit/chromium/src/BatteryClientImpl.cpp:77 >> +} > > Shouldn't we set m_controller = 0 ? Good catch! Done. Thanks.

Created attachment 136183 [details] Patch

Comment on attachment 136183 [details] Patch I noticed a crash when testing with the chromium side of the CL (http://codereview.chromium.org/10024013/). I have included a fix in this CL (in BatteryController::updateBatteryStatus) Thanks!

Comment on attachment 136183 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136183&action=review > Source/WebCore/Modules/battery/BatteryController.cpp:83 > + m_batteryStatus = status; status.release() See http://www.webkit.org/coding/RefPtr.html if you're unsure why. > Source/WebKit/chromium/public/WebView.h:458 > + // Battery status API support ------------------------------------------- I think we usually have a blank line after these section dividers. > Source/WebKit/chromium/public/WebView.h:459 > + virtual void updateBatteryStatus(const WebBatteryStatus&) { } Can you add a comment about what this function does?

Comment on attachment 136183 [details] Patch Attachment 136183 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12367780 New failing tests: fast/dom/navigator-detached-no-crash.html

Created attachment 136231 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 136235 [details] Patch

Comment on attachment 136183 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136183&action=review >> Source/WebCore/Modules/battery/BatteryController.cpp:83 >> + m_batteryStatus = status; > > status.release() > > See http://www.webkit.org/coding/RefPtr.html if you're unsure why. Thanks! That's rather helpful :) Done. >> Source/WebKit/chromium/public/WebView.h:458 >> + // Battery status API support ------------------------------------------- > > I think we usually have a blank line after these section dividers. Done. >> Source/WebKit/chromium/public/WebView.h:459 >> + virtual void updateBatteryStatus(const WebBatteryStatus&) { } > > Can you add a comment about what this function does? Done.

> fast/dom/navigator-detached-no-crash.html Did you solve this problem too?

Created attachment 136246 [details] Patch

(In reply to comment #28) > > fast/dom/navigator-detached-no-crash.html > > Did you solve this problem too? I hadn't. I failed at grokking the results in the zip. But this should now be fixed in the latest patchset. Sorry about that!

Comment on attachment 136246 [details] Patch Lets rock and roll.

Comment on attachment 136246 [details] Patch Clearing flags on attachment: 136246 Committed r113613: <http://trac.webkit.org/changeset/113613>

All reviewed patches have been landed. Closing bug.

This is crashing on all the chromium bots that have cycled so far. I'm rolling it out. Can you please re-land it once the crashes are fixed? You can dig into the output here: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fdom%2Fprototype-inheritance-2.html

Reopening to attach new patch.

Created attachment 136335 [details] Patch

Comment on attachment 136335 [details] Patch Included fix for the crash (in BatteryManager.cpp). The crash was caused by accessing the various battery properties before the browser sent any updates. The fix makes sure in such cases, it returns the appropriate values as specified in the spec.

Comment on attachment 136335 [details] Patch Clearing flags on attachment: 136335 Committed r113734: <http://trac.webkit.org/changeset/113734>

All reviewed patches have been landed. Closing bug.

Sorry to keep doing this, but I'm rolling it out again. I noticed a bunch of bizarre failures yesterday when this went in and I see them again today when it re-landed. Perhaps there is a memory stomper here? Or some other bad interaction, possibly with the filesystem? If for some reason the rollout doesn't fix them again, I'll reland this and try another theory. I'll keep you posted. I've got a win_layout trybot running this too. Maybe it'll give us a better stack trace. For the record, these are the failures: A bunch of chromium file-related browser_tests fail: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=browser_tests&tests=IndexedDBLayoutTest.TransactionTests%2CIndexedDBLayoutTest.BasicTests%2CIndexedDBLayoutTest.RegressionTests%2CWorkerLayoutTest.WorkerMessagePort%2CWorkerLayoutTest.WorkerContextMultiPort%2CFileSystemBrowserTestWithLowQuota.QuotaTest And a bunch of filesystem layout tests fail, timeout, or crash: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#master=ChromiumWebkit&tests=fast/filesystem/op-restricted-names.html,fast/files/workers/worker-read-file-sync.html,fast/filesystem/file-writer-abort-continue.html,fast/filesystem/file-writer-gc-blob.html,fast/filesystem/filesystem-reference.html,fast/filesystem/workers/file-writer-sync-truncate-extend.html,fast/filesystem/workers/file-writer-truncate-extend.html,fast/filesystem/workers/simple-persistent-sync.html,fast/filesystem/workers/simple-temporary.html,http/tests/filesystem/resolve-uri.html,http/tests/inspector-enabled/dom-storage-open.html,http/tests/inspector/console-xhr-logging-async.html,fast/doctypes/001.html,http/tests/inspector/indexeddb/database-data.html,http/tests/inspector/indexeddb/database-names.html,http/tests/inspector/indexeddb/database-structure.html,http/tests/inspector/indexeddb/resources-panel.html,http/tests/inspector/network/async-xhr-json-mime-type.html,http/tests/inspector/network/network-cachedresources-with-same-urls.html,http/tests/inspector/network/network-content-replacement-xhr.html,http/tests/local/formdata/send-form-data-with-sliced-file.html

Created attachment 136590 [details] Patch for landing

A clobber build seemed to fix things. So I'm trying this again. If I see issues, I'll try to clobber first. Maybe this patch requires a clobber.

Comment on attachment 136590 [details] Patch for landing Rejecting attachment 136590 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/12383630

Comment on attachment 136590 [details] Patch for landing Rejecting attachment 136590 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: 935a9025b9cee67f0e90951ed69b0a2 r113831 = f23ca47c08049274f267290ca2eaa52f03dd3323 r113832 = 4a2bce6958db5450873f96a2af84547d8d794e28 r113833 = ded8c2298470c8a98f7b5e29c77b1f16ee3079ea Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': could not connect to server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295 Died at Tools/Scripts/update-webkit line 164. Full output: http://queues.webkit.org/results/12379925

Comment on attachment 136590 [details] Patch for landing We're supposed to have code to catch when update fails and not cq- the patch... maybe that code is not working.

Comment on attachment 136590 [details] Patch for landing Clearing flags on attachment: 136590 Committed r113845: <http://trac.webkit.org/changeset/113845>

All reviewed patches have been landed. Closing bug.

It seems like this has caused a whole bunch of tests to crash on Chromium Windows: http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29/builds/25415 http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29/builds/25416

(In reply to comment #48) > It seems like this has caused a whole bunch of tests to crash on Chromium Windows: > http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29/builds/25415 > http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29/builds/25416 Yeah, I tihnk we confirmed that it requires a clobber. It'd be nice if someone looked into why.

Comment on attachment 136590 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=136590&action=review > Source/WebKit/chromium/public/WebBatteryStatusClient.h:35 > + virtual void startUpdating() = 0; why is this a *Client interface? these are commands. there are not notifications or policy questions. > Source/WebKit/chromium/public/WebViewClient.h:332 > + virtual WebBatteryStatusClient* batteryStatusClient() { return 0; } why is the battery status view-dependent? isn't there only one battery on the system? i would have expected an API more like the following: class WebKitPlatformSupport { ... virtual WebBatteryMonitor* batteryMonitor(); }; class WebBatteryMonitor { public: virtual void start(WebBatteryMonitorClient* client) = 0; virtual void stop() = 0; }; the Client would receive WebBatteryStatus objects.

(In reply to comment #50) > (From update of attachment 136590 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=136590&action=review > > > Source/WebKit/chromium/public/WebBatteryStatusClient.h:35 > > + virtual void startUpdating() = 0; > > why is this a *Client interface? these are commands. there are not notifications > or policy questions. > > > Source/WebKit/chromium/public/WebViewClient.h:332 > > + virtual WebBatteryStatusClient* batteryStatusClient() { return 0; } > > why is the battery status view-dependent? isn't there only one battery > on the system? > > i would have expected an API more like the following: > > class WebKitPlatformSupport { > ... > virtual WebBatteryMonitor* batteryMonitor(); > }; > > class WebBatteryMonitor { > public: > virtual void start(WebBatteryMonitorClient* client) = 0; > virtual void stop() = 0; > }; > > the Client would receive WebBatteryStatus objects. Hi! The approach in this CL closely follows some of the other similar APIs (WebDeviceOrientationClient for device-orientation events, for example). Given that, do you think it is reasonable that the current approach is sufficiently acceptable?