[chromium] Port test shell geolocation fixes to DRT
Created attachment 66743 [details] Patch
Pulls test shell fixes from http://codereview.chromium.org/3294007/show and http://codereview.chromium.org/3338008/show into DRT
Created attachment 66744 [details] Patch
NOTE patch set 1 does the callback async like test shell does, however this does not seem to be needed to I uploaded the simpler patchset 2 without this. Let me know which seems better....
thanks joth! looks good overall, just a couple of nits and one suggestion below. please, ask jorlow for a sanity check as well.. View in context: https://bugs.webkit.org/attachment.cgi?id=66744&action=prettypatch > LayoutTests/platform/chromium/drt_expectations.txt:174 > +//BUG_DRT LINUX : fast/dom/Geolocation/delayed-permission-allowed-for-multiple-requests.html = TEXT TIMEOUT remove these lines rather than comment? > WebKit/chromium/src/WebGeolocationServiceMock.cpp:145 > + // In addition to an instnace pointer, we need to keep the setPermission state typo, instance > WebKit/chromium/src/WebGeolocationServiceMock.cpp:149 > + static bool s_mockGeolocationPermissionIsSet; would it make sense to have a single enum {UNSET, ALLOWED, DENIED} instead of two bools? (In reply to comment #4) > NOTE patch set 1 does the callback async like test shell does, however this does not seem to be needed to I uploaded the simpler patchset 2 without this. Let me know which seems better....
LGTM > NOTE patch set 1 does the callback async like test shell does, however this does not seem to be needed > to I uploaded the simpler patchset 2 without this. Let me know which seems better.... There's certainly no need for the response to requestPermissionForFrame() to be asynchronous - WebCore::Geolocation is designed to handle this case. I think it's also safe for calls to layoutTestController.setMockGeolocationPermission() to cause synchronous calls straight through to WebCore::Geolocation, as that object will never make synchronous callbacks to script in response anyway. Other platforms have decided to make both cases asynchronous, however, to closer match the behaviour in 'real' use, where the browser responds to requestPermissionForFrame() asynchronously.
Created attachment 66867 [details] Patch
Thanks for the comments Marcus, Steve. I decided to keep DRT using the synchronous callbacks, as it works fine whereas I found async callbacks with >0 delay actually introduces new failures... This is now ready to go from my side. Jeremy, want to take a final pass and add it to the commit queue if OK? Cheers
Comment on attachment 66867 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=66867&action=prettypatch > WebKit/chromium/src/WebGeolocationServiceMock.cpp:231 > + m_pendingPermissionRequests.remove(i); Is this horribly slow? Maybe better to swap this with the last item? Or maybe it really doesn't matter since this is only for testing code? > WebKit/chromium/src/WebGeolocationServiceMock.cpp:243 > + it != pendingPermissionRequests.end(); ++it) { dont' wrap
View in context: https://bugs.webkit.org/attachment.cgi?id=66867&action=prettypatch > WebKit/chromium/src/WebGeolocationServiceMock.cpp:231 > + m_pendingPermissionRequests.remove(i); Horrible yes, slow probably not in practice as the normal case will be that the vector has zero or at most one item in it. So yes, I think correctness trumps speed as it's a test-code only corner case. Ah. Actually, given the if() on line 246, I could ditch this whole loop, and just let any stale entries work their way out naturally. Inconsistent with test shell, but perhaps simpler. > WebKit/chromium/src/WebGeolocationServiceMock.cpp:243 > + it != pendingPermissionRequests.end(); ++it) { oh yes. And line 196.
Created attachment 67169 [details] Unified patch, merged in unrollout of http://trac.webkit.org/changeset/66886 and fixes the expectations files
Patch 4 now fixes up the DRT and relands http://trac.webkit.org/changeset/66886 which was rolled out in http://trac.webkit.org/changeset/66892 due to said DRT failures. In addition, I'm now cleaning up the expectations files in this patch too, to make DRT (and test shell) run clean. This obsoletes the patch in https://bugs.webkit.org/show_bug.cgi?id=43480
*** Bug 43480 has been marked as a duplicate of this bug. ***
Comment on attachment 67169 [details] Unified patch, merged in unrollout of http://trac.webkit.org/changeset/66886 and fixes the expectations files This is rather hard to follow, as it seems to be an amalgamation of three bugs/patches. Do you need them all to go in one patch? If not, I think it would be much simpler to submit them separately. If they must go in as one, I think you should update the bugs by marking some as duplicates so that they all point to one bug. Then we can add a clear explanation to that bug of what's going on and make sure all the ChangeLog entries in this patch point to that same bug.
(In reply to comment #14) > (From update of attachment 67169 [details]) > This is rather hard to follow, as it seems to be an amalgamation of three bugs/patches. Do you need them all to go in one patch? If not, I think it would be much simpler to submit them separately. If they must go in as one, I think you should update the bugs by marking some as duplicates so that they all point to one bug. Then we can add a clear explanation to that bug of what's going on and make sure all the ChangeLog entries in this patch point to that same bug. Agree it's a bit confusing, but it is just a merge of 3 already reviewed patches. But yes, they need to go in at once, as the rollout happened because it broke DRT; landing them together ensures this won't happen again. Likewise, the expectations files need to be updated when the fixes land. The original bug https://bugs.webkit.org/show_bug.cgi?id=45112 was already closed when it landed the first time. (I'll add note to it about the rollout and link here for the re-land) I already marked https://bugs.webkit.org/show_bug.cgi?id=43480 as a duplicate of this one.
Comment on attachment 67169 [details] Unified patch, merged in unrollout of http://trac.webkit.org/changeset/66886 and fixes the expectations files View in context: https://bugs.webkit.org/attachment.cgi?id=67169&action=prettypatch > WebKit/chromium/src/WebGeolocationServiceMock.cpp:211 > + notifyPendingPermissions(); 4 spaces r=me
> But yes, they need to go in at once, as the rollout happened because it broke > DRT; landing them together ensures this won't happen again. Likewise, the > expectations files need to be updated when the fixes land. OK, fair enough. I think though that all the ChangeLogs should point to this single bug - I don't know of any other cases of a patch pointing to multiple bugs. Jeremy?
(In reply to comment #17) > > But yes, they need to go in at once, as the rollout happened because it broke > > DRT; landing them together ensures this won't happen again. Likewise, the > > expectations files need to be updated when the fixes land. > OK, fair enough. I think though that all the ChangeLogs should point to this single bug - I don't know of any other cases of a patch pointing to multiple bugs. Jeremy? This was all already reviewed. Technically it didn't need to even be given a bug or be reviewed, but Joth isn't a committer, so this way it can go through the commit queue.
(In reply to comment #18) > (In reply to comment #17) > > > But yes, they need to go in at once, as the rollout happened because it broke > > > DRT; landing them together ensures this won't happen again. Likewise, the > > > expectations files need to be updated when the fixes land. > > OK, fair enough. I think though that all the ChangeLogs should point to this single bug - I don't know of any other cases of a patch pointing to multiple bugs. Jeremy? > > This was all already reviewed. Technically it didn't need to even be given a bug or be reviewed, but Joth isn't a committer, so this way it can go through the commit queue. And there is a lot of precedent for landing multiple things at once.
Created attachment 67179 [details] jorlows and steve's comments; fixup ChangeLogs
(In reply to comment #20) > Created an attachment (id=67179) [details] > jorlows and steve's comments; fixup ChangeLogs I've left in the reference to the original bug where needed, but made this one the "main" bug for each ChangeLog entry and removed all reference to obsolete https://bugs.webkit.org/show_bug.cgi?id=43480 Hope that works... please commit-queue it. Cheers!
Comment on attachment 67179 [details] jorlows and steve's comments; fixup ChangeLogs rubber stamp
Comment on attachment 67179 [details] jorlows and steve's comments; fixup ChangeLogs Rejecting patch 67179 from commit-queue. Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Jeremy Orlow', u'--force']" exit_code: 1 Last 500 characters of output: locationServiceChromium.cpp patching file WebCore/platform/chromium/GeolocationServiceChromium.h patching file WebKit/chromium/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file WebKit/chromium/public/WebGeolocationService.h patching file WebKit/chromium/public/WebGeolocationServiceBridge.h patching file WebKit/chromium/public/WebGeolocationServiceMock.h patching file WebKit/chromium/src/WebGeolocationServiceBridgeImpl.cpp patching file WebKit/chromium/src/WebGeolocationServiceMock.cpp Full output: http://queues.webkit.org/results/3952350
Created attachment 67402 [details] Rebase patch after drt_expectations reorg
Comment on attachment 67402 [details] Rebase patch after drt_expectations reorg rebased
Comment on attachment 67402 [details] Rebase patch after drt_expectations reorg Clearing flags on attachment: 67402 Committed r67387: <http://trac.webkit.org/changeset/67387>
All reviewed patches have been landed. Closing bug.