Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2565183002:
Add a nextLocalAncestor helper for Fullscreen (Closed)
DescriptionAdd a nextLocalAncestor helper for Fullscreen
Refactoring only, no observable change intended.
A test is added that fails if the continue statement were dropped, as
apparently there was no coverage for this.
Note: This CL is also a test for the WPT export process.
BUG=402376
Committed: https://crrev.com/dd3989c829ddc1d3d36306143261278a20804229
Cr-Commit-Position: refs/heads/master@{#438076}
Patch Set 1 #
Total comments: 2
Patch Set 2 : early return #
Total comments: 3
Patch Set 3 : add continue and test that would break without it #
Messages
Total messages: 39 (26 generated)
The CQ bit was checked by foolip@chromium.org 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 ========== Simplify topmostLocalAncestor BUG=402376 ========== to ========== Add a nextLocalAncestor helper for Fullscreen Refactoring only, no observable change intended. ==========
foolip@chromium.org changed reviewers: + alexmos@chromium.org
PTAL and CQ
Description was changed from ========== Add a nextLocalAncestor helper for Fullscreen Refactoring only, no observable change intended. ========== to ========== Add a nextLocalAncestor helper for Fullscreen Refactoring only, no observable change intended. BUG=402376 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
foolip@chromium.org changed reviewers: + mlamouri@chromium.org
mlamouri@, may you could take a look at this too? (https://codereview.chromium.org/2557943002 includes the changes and will need rebasing)
The CQ bit was checked by foolip@chromium.org 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...
lgtm but would it be reasonable to add the notion of closest parent ancestor to at least Frame? https://codereview.chromium.org/2565183002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right): https://codereview.chromium.org/2565183002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Fullscreen.cpp:216: return nullptr; Feel free to ignore but I think it would read better as: ``` if (!document.frame()) return nullptr; LocalFrame* next = nextLocalAncestor(*document.frame())) return next ? next->document() : nullptr; ```
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/12 13:36:56, mlamouri wrote:
> lgtm but would it be reasonable to add the notion of closest parent ancestor
to
> at least Frame?
Do you mean something like Frame* Frame::parent() { return tree().parent(); }?
That would make some of the code shorter, but currently all the traversal stuff
is inside FrameTree and I'm guessing this was intentional and didn't start out
like that.
(If you mean a nextLocalAncestor-like helper in Frame/FrameTree, then I'm
inclined to keep it in Fullscreen, because virtually every use of it is
problematic and decorated with OOPIF TODOs. It would be hard to use this helper
in a way that isn't problematic for OOPIF.)
https://codereview.chromium.org/2565183002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right): https://codereview.chromium.org/2565183002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Fullscreen.cpp:216: return nullptr; On 2016/12/12 13:36:56, mlamouri wrote: > Feel free to ignore but I think it would read better as: > ``` > if (!document.frame()) > return nullptr; > > LocalFrame* next = nextLocalAncestor(*document.frame())) > return next ? next->document() : nullptr; > ``` The ternary part of that was how I started out, but I worried about whether it might be null, and think the DCHECK there has some value. I'll go with consistent early return, which is a little bit longer, but will probably get compiled to roughly the same thing.
The CQ bit was checked by foolip@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2565183002/#ps20001 (title: "early return")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
(stopped CQ for the question below) https://codereview.chromium.org/2565183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right): https://codereview.chromium.org/2565183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Fullscreen.cpp:558: currentDoc = nextLocalAncestor(*currentDoc); Doesn't this need a "continue" to match the old behavior? Otherwise seems like currentDoc will get set to nullptr and drop out of the loop.
The CQ bit was unchecked by alexmos@chromium.org
https://codereview.chromium.org/2565183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right): https://codereview.chromium.org/2565183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Fullscreen.cpp:558: currentDoc = nextLocalAncestor(*currentDoc); On 2016/12/12 17:18:03, alexmos wrote: > Doesn't this need a "continue" to match the old behavior? Otherwise seems like > currentDoc will get set to nullptr and drop out of the loop. Oh my, good catch. Will see why no tests fail because of this bug.
The CQ bit was checked by foolip@chromium.org to run a CQ dry run
Description was changed from ========== Add a nextLocalAncestor helper for Fullscreen Refactoring only, no observable change intended. BUG=402376 ========== to ========== Add a nextLocalAncestor helper for Fullscreen Refactoring only, no observable change intended. A test is added that fails if the continue statement were dropped, as apparently there was no coverage for this. Note: This CL is also a test for the WPT export process. BUG=402376 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
foolip@chromium.org changed reviewers: + jeffcarp@chromium.org
alexmos@, can you take another look and review the test? jeffcarp@, this a test CL for export, but also has real changes, so there's a non-zero change of it being reverted, in which case fun things might happen. https://codereview.chromium.org/2565183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right): https://codereview.chromium.org/2565183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Fullscreen.cpp:558: currentDoc = nextLocalAncestor(*currentDoc); On 2016/12/12 17:27:44, foolip wrote: > On 2016/12/12 17:18:03, alexmos wrote: > > Doesn't this need a "continue" to match the old behavior? Otherwise seems > like > > currentDoc will get set to nullptr and drop out of the loop. > > Oh my, good catch. Will see why no tests fail because of this bug. I wrote a test and updated the description. The test would fail because one of the events when exiting would go missing.
LGTM. The test looks good - thanks for adding it!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by foolip@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2565183002/#ps40001 (title: "add continue and test that would break without it")
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": 40001, "attempt_start_ts": 1481609043487780,
"parent_rev": "77d538dca00d6c05a5941a766d23d57aedcc5c53", "commit_rev":
"6936da87f8723691b74dc9576420f4ec958bdfa1"}
Message was sent while issue was closed.
Description was changed from ========== Add a nextLocalAncestor helper for Fullscreen Refactoring only, no observable change intended. A test is added that fails if the continue statement were dropped, as apparently there was no coverage for this. Note: This CL is also a test for the WPT export process. BUG=402376 ========== to ========== Add a nextLocalAncestor helper for Fullscreen Refactoring only, no observable change intended. A test is added that fails if the continue statement were dropped, as apparently there was no coverage for this. Note: This CL is also a test for the WPT export process. BUG=402376 Review-Url: https://codereview.chromium.org/2565183002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add a nextLocalAncestor helper for Fullscreen Refactoring only, no observable change intended. A test is added that fails if the continue statement were dropped, as apparently there was no coverage for this. Note: This CL is also a test for the WPT export process. BUG=402376 Review-Url: https://codereview.chromium.org/2565183002 ========== to ========== Add a nextLocalAncestor helper for Fullscreen Refactoring only, no observable change intended. A test is added that fails if the continue statement were dropped, as apparently there was no coverage for this. Note: This CL is also a test for the WPT export process. BUG=402376 Committed: https://crrev.com/dd3989c829ddc1d3d36306143261278a20804229 Cr-Commit-Position: refs/heads/master@{#438076} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/dd3989c829ddc1d3d36306143261278a20804229 Cr-Commit-Position: refs/heads/master@{#438076} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
