Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2731953003:
[DeviceService] Replace vibration_browsertest.cc with layout tests. (Closed)
|
Created:
3 years, 9 months ago by leonhsl(Using Gerrit) Modified:
3 years, 9 months ago CC:
chromium-reviews, mlamouri+watch-blink_chromium.org, mvanouwerkerk+watch_chromium.org, blink-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[DeviceService] Replace vibration_browsertest.cc with layout tests.
vibration_browsertest.cc tests vibration integration between Blink and
the browser. It does this by injecting a fake VibrationManager impl
into RenderFrameHost's InterfaceRegistry.
However, this blocks us to port VibrationManager to be hosted in
Device Service because after that there is no such mechanism to do this.
So this CL replaces these browsertests with layout tests, with a TODO
to add 'main frame reload' tests later, because for now we consider that
'sub frame reload' tests have actually covered what we want to verify
for lifecycle of NavigatorVibration.
BUG=686692
TEST=blink_tests
Review-Url: https://codereview.chromium.org/2731953003
Cr-Commit-Position: refs/heads/master@{#457710}
Committed: https://chromium.googlesource.com/chromium/src/+/5033377133364937998758c5b77bf63e95824e73
Patch Set 1 #
Total comments: 1
Patch Set 2 #
Total comments: 13
Patch Set 3 : Add case: reload subframe #
Total comments: 2
Patch Set 4 : Add case: destroy subframe #Patch Set 5 : Add TODO #
Total comments: 14
Patch Set 6 : Address comments from Tim #Patch Set 7 : 'git cl format --js' for js/html #
Messages
Total messages: 78 (49 generated)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
leon.han@intel.com changed reviewers: + blundell@chromium.org, mvanouwerkerk@chromium.org
Hi, would you PTAL at ps#1 firstly? So that I can know whether I'm on the correct direction now. Thanks! # ps#1 just has an initial test case, will continue to cover all test cases of vibration_browsertest.cc in this CL later. https://codereview.chromium.org/2731953003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/vibration/resources/vibration-helpers.js (right): https://codereview.chromium.org/2731953003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/vibration/resources/vibration-helpers.js:18: window.postMessage(milliseconds, '*'); I'm not sure whether this is the best practice to notify that mojo function got called, in layout tests.
Hi Han Leon, This looks like a good start! Could you complete the first layout test (i.e., add the expected file(s) and anything else that's required), and then ping again here? At that point we can have Michael or Tim take a look (I am very much not an expert in writing layout tests).
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
leon.han@intel.com changed reviewers: + timvolodine@chromium.org
I'm also a newbie in writing layout tests.. And I suppose we do not need *_expected.txt files because we are using assert_* here, currently these newly added test cases are being executed actually by trybots. Maybe Michael and Tim could teach me more :) Thanks! https://codereview.chromium.org/2731953003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/vibration/mock-vibration.html (right): https://codereview.chromium.org/2731953003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/vibration/mock-vibration.html:16: // Cover test case in vibration_browsertest.cc: Vibrate This comment is to make review easier, will delete after review done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Tim, Could you take the lead on reviewing this CL? Han Leon is looking to add layout tests of vibration that exercise the same client-side functionality of as the existing vibration_browsertest.cc (as the linked bug describes, we need to eliminate that browsertest, so we're looking to add test coverage to replace it). I'm not at all experienced in writing layout tests. Presumably these vibration layout tests should be very similar to the battery ones. Thanks, Colin
On 2017/03/08 17:42:18, blundell wrote: > Hi Tim, > > Could you take the lead on reviewing this CL? Han Leon is looking to add layout > tests of vibration that exercise the same client-side functionality of as the > existing vibration_browsertest.cc (as the linked bug describes, we need to > eliminate that browsertest, so we're looking to add test coverage to replace > it). I'm not at all experienced in writing layout tests. Presumably these > vibration layout tests should be very similar to the battery ones. > > Thanks, > > Colin BTW, the CL is not yet complete at this point. Han Leon is looking for feedback on whether the basic approach and structure that he's set up is sound, after which he'll add the remaining layout tests.
On 2017/03/08 17:43:26, blundell wrote: > On 2017/03/08 17:42:18, blundell wrote: > > Hi Tim, > > > > Could you take the lead on reviewing this CL? Han Leon is looking to add > layout > > tests of vibration that exercise the same client-side functionality of as the > > existing vibration_browsertest.cc (as the linked bug describes, we need to > > eliminate that browsertest, so we're looking to add test coverage to replace > > it). I'm not at all experienced in writing layout tests. Presumably these > > vibration layout tests should be very similar to the battery ones. > > > > Thanks, > > > > Colin > > BTW, the CL is not yet complete at this point. Han Leon is looking for feedback > on whether the basic approach and structure that he's set up is sound, after > which he'll add the remaining layout tests. right, I was just looking at this CL, more to follow )
Structure looks fine to me. Thanks for adding these Han Leon. Some initial comments below. https://codereview.chromium.org/2731953003/diff/20001/content/browser/vibrati... File content/browser/vibration_browsertest.cc (left): https://codereview.chromium.org/2731953003/diff/20001/content/browser/vibrati... content/browser/vibration_browsertest.cc:140: CancelVibrationFromMainFrameWhenMainFrameIsReloaded) { Are the main frame and sub frame tests below going to be covered in the layout tests? https://codereview.chromium.org/2731953003/diff/20001/content/browser/vibrati... content/browser/vibration_browsertest.cc:143: ASSERT_TRUE(NavigateToURL(shell(), GetTestUrl(".", "simple_page.html"))); these htmls are probably used for other tests, would be good to double check, otherwise can remove as well.. https://codereview.chromium.org/2731953003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/vibration/mock-vibration.html (right): https://codereview.chromium.org/2731953003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/vibration/mock-vibration.html:1: <!DOCTYPE html> does it make sense to call this file mock-vibration.html? maybe just vibration.html? https://codereview.chromium.org/2731953003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/vibration/mock-vibration.html:4: <script src="../resources/testharness-helpers.js"></script> is this needed? https://codereview.chromium.org/2731953003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/vibration/mock-vibration.html:10: nit: maybe add if (!window.testRunner) debug('This test cannot be run without the TestRunner'); not really required, but will provide some feedback if not in test mode https://codereview.chromium.org/2731953003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/vibration/mock-vibration.html:14: }, 'VibrationManager Mojo bindings and mock interfaces are available.'); nit: '.. are available to tests.' (for consistency with other tests)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Uploaded ps#3, PTAnL with my inline questions.. Thanks! https://codereview.chromium.org/2731953003/diff/20001/content/browser/vibrati... File content/browser/vibration_browsertest.cc (left): https://codereview.chromium.org/2731953003/diff/20001/content/browser/vibrati... content/browser/vibration_browsertest.cc:140: CancelVibrationFromMainFrameWhenMainFrameIsReloaded) { On 2017/03/08 19:38:31, timvolodine wrote: > Are the main frame and sub frame tests below going to be covered in the layout > tests? Yeah this CL aims to cover all these tests. I added one for CancelVibrationFromSubFrameWhenSubFrameIsReloaded in ps#3. But I have no idea how to handle main frame reload case, because main frame reload will destroy everything of current html page so we can not monitor the result of vibration cancel. Any hints? Thanks! https://codereview.chromium.org/2731953003/diff/20001/content/browser/vibrati... content/browser/vibration_browsertest.cc:143: ASSERT_TRUE(NavigateToURL(shell(), GetTestUrl(".", "simple_page.html"))); On 2017/03/08 19:38:31, timvolodine wrote: > these htmls are probably used for other tests, would be good to double check, > otherwise can remove as well.. Checked around and found out that they are also used by other content browser tests. https://codereview.chromium.org/2731953003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/vibration/mock-vibration.html (right): https://codereview.chromium.org/2731953003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/vibration/mock-vibration.html:1: <!DOCTYPE html> On 2017/03/08 19:38:31, timvolodine wrote: > does it make sense to call this file mock-vibration.html? maybe just > vibration.html? Done. https://codereview.chromium.org/2731953003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/vibration/mock-vibration.html:4: <script src="../resources/testharness-helpers.js"></script> On 2017/03/08 19:38:31, timvolodine wrote: > is this needed? Yeah not necessary, removed. https://codereview.chromium.org/2731953003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/vibration/mock-vibration.html:10: On 2017/03/08 19:38:31, timvolodine wrote: > nit: maybe add > if (!window.testRunner) > debug('This test cannot be run without the TestRunner'); > > not really required, but will provide some feedback if not in test mode Done. https://codereview.chromium.org/2731953003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/vibration/mock-vibration.html:14: }, 'VibrationManager Mojo bindings and mock interfaces are available.'); On 2017/03/08 19:38:31, timvolodine wrote: > nit: '.. are available to tests.' (for consistency with other tests) Done. https://codereview.chromium.org/2731953003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/vibration/vibration-iframe.html (right): https://codereview.chromium.org/2731953003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/vibration/vibration-iframe.html:19: iframe.src = 'resources/vibrate-from-iframe.html'; This test case is created by mimicking nfc layout tests, but I can't get it PASS locally, error message: ' [1/1] vibration/vibration-iframe.html failed unexpectedly (test timed out) ' Because there has no any other detail error information, after tried several times, I still have no idea what is wrong.. Am I doing something stupid here? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2731953003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/vibration/vibration-iframe.html (right): https://codereview.chromium.org/2731953003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/vibration/vibration-iframe.html:19: iframe.src = 'resources/vibrate-from-iframe.html'; On 2017/03/09 08:34:10, leonhsl wrote: > This test case is created by mimicking nfc layout tests, but I can't get it PASS > locally, error message: > ' > [1/1] vibration/vibration-iframe.html failed unexpectedly (test timed out) > ' > Because there has no any other detail error information, after tried several > times, I still have no idea what is wrong.. Am I doing something stupid here? > Thanks! What I tend to do in cases like this is simple printf debugging: Add console.log() messages throughout your JS to narrow down where the behavior is deviating from what you expect. The most likely explanation seems that you're hanging at some point, so somewhere along the line a message isn't getting sent (or isn't getting processed) in order to move things along as you're expecting. That's just a hypothesis though.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Uploaded ps#4, I've confirmed PASS locally and all trybots are green.
Now the question is how could we handle bellowing 2 cases about 'main frame
reload', unlike 'sub frame reload', 'main frame reload' will destroy the
javascript environment of current page so we can't monitor the result.. Are
there any global places where we can put the result which we can still check
even after page reload? Thanks!
CancelVibrationFromMainFrameWhenMainFrameIsReloaded
CancelVibrationFromSubFrameWhenMainFrameIsReloaded
On 2017/03/10 09:29:45, leonhsl wrote: > Uploaded ps#4, I've confirmed PASS locally and all trybots are green. > Now the question is how could we handle bellowing 2 cases about 'main frame > reload', unlike 'sub frame reload', 'main frame reload' will destroy the > javascript environment of current page so we can't monitor the result.. Are > there any global places where we can put the result which we can still check > even after page reload? Thanks! > > CancelVibrationFromMainFrameWhenMainFrameIsReloaded > CancelVibrationFromSubFrameWhenMainFrameIsReloaded Hi Tim, do you have any insight here?
On 2017/03/13 15:49:21, blundell wrote: > On 2017/03/10 09:29:45, leonhsl wrote: > > Uploaded ps#4, I've confirmed PASS locally and all trybots are green. > > Now the question is how could we handle bellowing 2 cases about 'main frame > > reload', unlike 'sub frame reload', 'main frame reload' will destroy the > > javascript environment of current page so we can't monitor the result.. Are > > there any global places where we can put the result which we can still check > > even after page reload? Thanks! > > > > CancelVibrationFromMainFrameWhenMainFrameIsReloaded > > CancelVibrationFromSubFrameWhenMainFrameIsReloaded > > Hi Tim, do you have any insight here? Hmm good question, looks like this would be the first such use case.. Service workers come to mind as they would outlive the main frame, not sure if there are other options. Maybe raise the issue on the mojo mailinglist? Otherwise to not block on this perhaps try to address the mainframe tests in a follow-up patch..
On 2017/03/13 17:52:24, timvolodine wrote: > On 2017/03/13 15:49:21, blundell wrote: > > On 2017/03/10 09:29:45, leonhsl wrote: > > > Uploaded ps#4, I've confirmed PASS locally and all trybots are green. > > > Now the question is how could we handle bellowing 2 cases about 'main frame > > > reload', unlike 'sub frame reload', 'main frame reload' will destroy the > > > javascript environment of current page so we can't monitor the result.. Are > > > there any global places where we can put the result which we can still check > > > even after page reload? Thanks! > > > > > > CancelVibrationFromMainFrameWhenMainFrameIsReloaded > > > CancelVibrationFromSubFrameWhenMainFrameIsReloaded > > > > Hi Tim, do you have any insight here? > > Hmm good question, looks like this would be the first such use case.. Service > workers come to mind as they would outlive the main frame, not sure if there are > other options. Maybe raise the issue on the mojo mailinglist? Otherwise to not > block on this perhaps try to address the mainframe tests in a follow-up patch.. Hi, Tim, Colin, Thank you! I've started this https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/-s10gKcYJcI , hope some magic could work for us :)
On 2017/03/14 03:01:49, leonhsl wrote: > On 2017/03/13 17:52:24, timvolodine wrote: > > On 2017/03/13 15:49:21, blundell wrote: > > > On 2017/03/10 09:29:45, leonhsl wrote: > > > > Uploaded ps#4, I've confirmed PASS locally and all trybots are green. > > > > Now the question is how could we handle bellowing 2 cases about 'main > frame > > > > reload', unlike 'sub frame reload', 'main frame reload' will destroy the > > > > javascript environment of current page so we can't monitor the result.. > Are > > > > there any global places where we can put the result which we can still > check > > > > even after page reload? Thanks! > > > > > > > > CancelVibrationFromMainFrameWhenMainFrameIsReloaded > > > > CancelVibrationFromSubFrameWhenMainFrameIsReloaded > > > > > > Hi Tim, do you have any insight here? > > > > Hmm good question, looks like this would be the first such use case.. Service > > workers come to mind as they would outlive the main frame, not sure if there > are > > other options. Maybe raise the issue on the mojo mailinglist? Otherwise to not > > block on this perhaps try to address the mainframe tests in a follow-up > patch.. > > Hi, Tim, Colin, Thank you! I've started this > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/-s10gKcYJcI > , hope some magic could work for us :) I suggest that you open a bug and we proceed with this CL as-is as Tim suggested.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Uploaded ps#5 which adds a TODO and deletes useless comments, PTAnL, Thanks. And, I'm confused about the '*' in third_party/WebKit/LayoutTests/OWNERS, does it mean any one OWNER of blink or just everyone?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/14 10:03:46, leonhsl wrote: > Uploaded ps#5 which adds a TODO and deletes useless comments, PTAnL, Thanks. > > And, I'm confused about the '*' in third_party/WebKit/LayoutTests/OWNERS, does > it mean any one OWNER of blink or just everyone? "*" means anyone at all.
On 2017/03/14 12:16:12, blundell wrote: > On 2017/03/14 10:03:46, leonhsl wrote: > > Uploaded ps#5 which adds a TODO and deletes useless comments, PTAnL, Thanks. > > > > And, I'm confused about the '*' in third_party/WebKit/LayoutTests/OWNERS, does > > it mean any one OWNER of blink or just everyone? > > "*" means anyone at all. Hi Colin, got it, Thanks! Hi Tim, would you please help do an overall review firstly? I'll add //content OWNER after you are OK :)
Thanks Han Leon! (BTW I have to admit I am slightly confused should I call you "Han", "Leon" or "Han Leon"?) a few comments below, otherwise looks reasonable to me :) https://codereview.chromium.org/2731953003/diff/100001/content/browser/vibrat... File content/browser/vibration_browsertest.cc (left): https://codereview.chromium.org/2731953003/diff/100001/content/browser/vibrat... content/browser/vibration_browsertest.cc:140: CancelVibrationFromMainFrameWhenMainFrameIsReloaded) { so what's the plan with the main frame tests? keep them for now? https://codereview.chromium.org/2731953003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/vibration/vibration-iframe.html (right): https://codereview.chromium.org/2731953003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/vibration/vibration-iframe.html:51: }, 'Iframe reload would cancel the vibration started by it before.'); could this be formulated as an affirmation? e.g. 'Iframe reload cancels vibration started by it before' https://codereview.chromium.org/2731953003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/vibration/vibration-iframe.html:67: iframe.contentWindow.postMessage('Ready', '*'); if the alignment here correct? a bit hard to read https://codereview.chromium.org/2731953003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/vibration/vibration-iframe.html:89: }, 'Iframe destroy would cancel the vibration started by it before.'); same here https://codereview.chromium.org/2731953003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/vibration/vibration.html (right): https://codereview.chromium.org/2731953003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/vibration/vibration.html:34: }, 'navigator.vibrate(1234) can trigger MockVibrationManager vibrate(1234) correctly.'); nit: seems better: "can trigger" -> "triggers" also, probably even not necessary to mention MockVibrationManager, e.g. "navigator.vibrate(1234) triggers vibration correctly" up to you though how to formulate, what you prefer ) https://codereview.chromium.org/2731953003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/vibration/vibration.html:51: }, 'navigator.vibrate(0) can trigger MockVibrationManager cancel() correctly.'); also here
Patchset #6 (id:120001) has been deleted
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Tim a lot for kindly review! Uploaded ps#6, PTAnL. I'm Chinese so 'Han' is family name and 'Leon' is 'English Name' which I suppose is easier to be called than my Chinese first name :-) https://codereview.chromium.org/2731953003/diff/100001/content/browser/vibrat... File content/browser/vibration_browsertest.cc (left): https://codereview.chromium.org/2731953003/diff/100001/content/browser/vibrat... content/browser/vibration_browsertest.cc:140: CancelVibrationFromMainFrameWhenMainFrameIsReloaded) { On 2017/03/15 18:20:18, timvolodine wrote: > so what's the plan with the main frame tests? keep them for now? I added a TODO in vibration-iframe.html for the 2 main frame tests. And for now I want to delete this file completely so that we can proceed to port VibrationManager to be hosted in Device Service. https://codereview.chromium.org/2731953003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/vibration/vibration-iframe.html (right): https://codereview.chromium.org/2731953003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/vibration/vibration-iframe.html:51: }, 'Iframe reload would cancel the vibration started by it before.'); On 2017/03/15 18:20:18, timvolodine wrote: > could this be formulated as an affirmation? e.g. > 'Iframe reload cancels vibration started by it before' Done. Seems 'git cl format' does not work for js/html files, are there any other tools we can use? Thanks! https://codereview.chromium.org/2731953003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/vibration/vibration-iframe.html:67: iframe.contentWindow.postMessage('Ready', '*'); On 2017/03/15 18:20:18, timvolodine wrote: > if the alignment here correct? a bit hard to read Done. https://codereview.chromium.org/2731953003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/vibration/vibration-iframe.html:89: }, 'Iframe destroy would cancel the vibration started by it before.'); On 2017/03/15 18:20:18, timvolodine wrote: > same here Done. https://codereview.chromium.org/2731953003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/vibration/vibration.html (right): https://codereview.chromium.org/2731953003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/vibration/vibration.html:34: }, 'navigator.vibrate(1234) can trigger MockVibrationManager vibrate(1234) correctly.'); On 2017/03/15 18:20:18, timvolodine wrote: > nit: seems better: "can trigger" -> "triggers" > > also, probably even not necessary to mention MockVibrationManager, e.g. > "navigator.vibrate(1234) triggers vibration correctly" > up to you though how to formulate, what you prefer ) Done. https://codereview.chromium.org/2731953003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/vibration/vibration.html:51: }, 'navigator.vibrate(0) can trigger MockVibrationManager cancel() correctly.'); On 2017/03/15 18:20:18, timvolodine wrote: > also here Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Naive question: Is there any meaningful distinction between testing behavior on subframe destruction and testing behavior on main frame destruction in these layout tests? My reasoning is: By testing behavior on subframe destruction we've verified that NavigatorVibration takes the right action on being notified that its execution context has gone away. It seems like the only additional verification that we'd get from also testing behavior on main frame destruction is the verification that when the main frame is destroyed, NavigatorVibration indeed gets the notification that its execution context has gone away. The latter seems like a basic sanity property of ContextLifecycleObserver and thus outside the scope of these particular tests.
Thanks Leon, Maybe try formatting js files as mentioned below. Also would be good to add some description for this issue (which is currently empty and just points to the bug): something about 'why' and 'how/what'. https://codereview.chromium.org/2731953003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/vibration/vibration-iframe.html (right): https://codereview.chromium.org/2731953003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/vibration/vibration-iframe.html:51: }, 'Iframe reload would cancel the vibration started by it before.'); On 2017/03/16 06:32:42, leonhsl wrote: > On 2017/03/15 18:20:18, timvolodine wrote: > > could this be formulated as an affirmation? e.g. > > 'Iframe reload cancels vibration started by it before' > > Done. > Seems 'git cl format' does not work for js/html files, are there any other tools > we can use? Thanks! yes try 'git cl format --js'?
On 2017/03/16 10:46:20, blundell wrote: > Naive question: Is there any meaningful distinction between testing behavior on > subframe destruction and testing behavior on main frame destruction in these > layout tests? > > My reasoning is: By testing behavior on subframe destruction we've verified that > NavigatorVibration takes the right action on being notified that its execution > context has gone away. It seems like the only additional verification that we'd > get from also testing behavior on main frame destruction is the verification > that when the main frame is destroyed, NavigatorVibration indeed gets the > notification that its execution context has gone away. The latter seems like a > basic sanity property of ContextLifecycleObserver and thus outside the scope of > these particular tests. Sounds reasonable, I think Michael (mvanouwerkerk@) added the original main frame tests so if there are no objections from his side I think I am fine with the patch doing the subframe tests now. So lgtm here, but please check with Michael on the potential special cases regarding this.
lgtm
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [DeviceService] Replace vibration_browsertest.cc with layout tests. BUG=686692 TEST=blink_tests ========== to ========== [DeviceService] Replace vibration_browsertest.cc with layout tests. vibration_browsertest.cc tests vibration integration between Blink and the browser. It does this by injecting a fake VibrationManager impl into RenderFrameHost's InterfaceRegistry. However, this blocks us to port VibrationManager to be hosted in Device Service because after that there is no such mechanism to do this. So this CL replaces these browsertests with layout tests, with a TODO to add 'main frame reload' tests, because fow now we consider that 'sub frame reload' tests have actually covered what we want to verify for lifecycle of NavigatorVibration. BUG=686692 TEST=blink_tests ==========
Thanks all! Uploaded ps#7 to format js/html files. https://codereview.chromium.org/2731953003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/vibration/vibration-iframe.html (right): https://codereview.chromium.org/2731953003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/vibration/vibration-iframe.html:51: }, 'Iframe reload would cancel the vibration started by it before.'); On 2017/03/16 15:32:21, timvolodine wrote: > On 2017/03/16 06:32:42, leonhsl wrote: > > On 2017/03/15 18:20:18, timvolodine wrote: > > > could this be formulated as an affirmation? e.g. > > > 'Iframe reload cancels vibration started by it before' > > > > Done. > > Seems 'git cl format' does not work for js/html files, are there any other > tools > > we can use? Thanks! > > yes try 'git cl format --js'? > Done and Thanks!
leon.han@intel.com changed reviewers: + kinuko@chromium.org
+kinuko@, need your approval again for removal of content/browser/vibration_browsertest.cc, Thanks:)
Description was changed from ========== [DeviceService] Replace vibration_browsertest.cc with layout tests. vibration_browsertest.cc tests vibration integration between Blink and the browser. It does this by injecting a fake VibrationManager impl into RenderFrameHost's InterfaceRegistry. However, this blocks us to port VibrationManager to be hosted in Device Service because after that there is no such mechanism to do this. So this CL replaces these browsertests with layout tests, with a TODO to add 'main frame reload' tests, because fow now we consider that 'sub frame reload' tests have actually covered what we want to verify for lifecycle of NavigatorVibration. BUG=686692 TEST=blink_tests ========== to ========== [DeviceService] Replace vibration_browsertest.cc with layout tests. vibration_browsertest.cc tests vibration integration between Blink and the browser. It does this by injecting a fake VibrationManager impl into RenderFrameHost's InterfaceRegistry. However, this blocks us to port VibrationManager to be hosted in Device Service because after that there is no such mechanism to do this. So this CL replaces these browsertests with layout tests, with a TODO to add 'main frame reload' tests later, because for now we consider that 'sub frame reload' tests have actually covered what we want to verify for lifecycle of NavigatorVibration. BUG=686692 TEST=blink_tests ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by leon.han@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from timvolodine@chromium.org, mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2731953003/#ps160001 (title: "'git cl format --js' for js/html")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1489736381708250,
"parent_rev": "a6660c0cd2517e9da95f099211042d0c39fe8055", "commit_rev":
"5033377133364937998758c5b77bf63e95824e73"}
Message was sent while issue was closed.
Description was changed from ========== [DeviceService] Replace vibration_browsertest.cc with layout tests. vibration_browsertest.cc tests vibration integration between Blink and the browser. It does this by injecting a fake VibrationManager impl into RenderFrameHost's InterfaceRegistry. However, this blocks us to port VibrationManager to be hosted in Device Service because after that there is no such mechanism to do this. So this CL replaces these browsertests with layout tests, with a TODO to add 'main frame reload' tests later, because for now we consider that 'sub frame reload' tests have actually covered what we want to verify for lifecycle of NavigatorVibration. BUG=686692 TEST=blink_tests ========== to ========== [DeviceService] Replace vibration_browsertest.cc with layout tests. vibration_browsertest.cc tests vibration integration between Blink and the browser. It does this by injecting a fake VibrationManager impl into RenderFrameHost's InterfaceRegistry. However, this blocks us to port VibrationManager to be hosted in Device Service because after that there is no such mechanism to do this. So this CL replaces these browsertests with layout tests, with a TODO to add 'main frame reload' tests later, because for now we consider that 'sub frame reload' tests have actually covered what we want to verify for lifecycle of NavigatorVibration. BUG=686692 TEST=blink_tests Review-Url: https://codereview.chromium.org/2731953003 Cr-Commit-Position: refs/heads/master@{#457710} Committed: https://chromium.googlesource.com/chromium/src/+/5033377133364937998758c5b77b... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001) as https://chromium.googlesource.com/chromium/src/+/5033377133364937998758c5b77b...
Message was sent while issue was closed.
On 2017/03/17 07:45:15, commit-bot: I haz the power wrote: > Committed patchset #7 (id:160001) as > https://chromium.googlesource.com/chromium/src/+/5033377133364937998758c5b77b... This was really nice work, thanks! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
