Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2751833002:
Clean up DocumentWriter creation's FrameLoader interaction (Closed)
|
Created:
3 years, 9 months ago by Nate Chapin Modified:
3 years, 9 months ago CC:
blink-reviews, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionClean up DocumentWriter creation's FrameLoader interaction
There are multiple methods in FrameLoader that take part in the commit
and Document creation sequence, with less-than-clear purposes. Also,
these methods make little-to-no use of FrameLoader's members, instead
primarily interacting with Document, LocalFrameClient, and the
DocumentLoader that is driving the commit. Move a bunch of this logic
to DocumentLoader, since it's all tied to the loading of a specific
Document anyway.
BUG=
Review-Url: https://codereview.chromium.org/2751833002
Cr-Commit-Position: refs/heads/master@{#458549}
Committed: https://chromium.googlesource.com/chromium/src/+/ace017f26fd8cb8d123238e41c2f0e11f5daacd6
Patch Set 1 #Patch Set 2 : Clean up DocumentWriter creation's FrameLoader interaction #Patch Set 3 : Compile fix #Patch Set 4 : Clean up DocumentWriter creation's FrameLoader interaction #
Total comments: 13
Patch Set 5 : Rebase + nits #
Messages
Total messages: 40 (32 generated)
The CQ bit was checked by japhet@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by japhet@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 japhet@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...
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 japhet@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_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 japhet@chromium.org
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by japhet@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by japhet@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 ========== Clean up DocumentWriter creation's FrameLoader interaction BUG= ========== to ========== Clean up DocumentWriter creation's FrameLoader interaction There are multiple methods in FrameLoader that take part in the commit and Document creation sequence, with less-than-clear purposes. Also, these methods make little-to-no use of FrameLoader's members, instead primarily interacting with Document, LocalFrameClient, and the DocumentLoader that is driving the commit. Move a bunch of this logic to DocumentLoader, since it's all tied to the loading of a specific Document anyway. BUG= ==========
japhet@chromium.org changed reviewers: + mkwst@chromium.org, yhirano@chromium.org
DocumentLoader/FrameLoader cleanup! https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:919: frameLoader().dispatchDidClearDocumentOfWindowObject(); This single caller appears to meet the requirements of both the standard commit path and the javascript: url "navigation" path, instead of having separate calls in didInstallNewDocument and receivedFirstData in FrameLoader. https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/DocumentLoader.h (left): https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.h:219: static DocumentWriter* createWriterFor(const DocumentInit&, This method (1) does enough DocumentLoader-related stuff that I think non-static is better, (2) does more than creating a DocumentWriter, renamed accordingly. https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left): https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:478: if (m_stateMachine.creatingInitialEmptyDocument()) This early-exit, and the second half of the function below, moved to DocumentLoader::didCommitNavigation. Hope to move the rest of receivedFirstData() in a followup. https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:526: takeObjectSnapshot(); Removed. I don't think this snapshot is adding a lot, given that we also snapshot in clear() and prepareForCommit(), which are called shortly before this. https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:533: m_frame->document()->setReadyState(Document::Loading); All this stuff moved to DocumentLoader::didInstallNewDocument() https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:542: if (m_provisionalItem && Like the remainder of receivedFirstData(), I want to move this clause to DocumentLoader as part of a history logic refactor followup. https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:549: void FrameLoader::didBeginDocument() { Most of this moved to DocumentLoader::didInstallNewDocument, with a lot fewer unnecessary null-checks. The feature policy logic got its own helper due to ordering dependencies.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Sorry for the delayed response. I think this looks like a pretty reasonable refactoring, and the bots agree with me. One nit, otherwise LGTM. https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/DocumentLoader.h (left): https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.h:219: static DocumentWriter* createWriterFor(const DocumentInit&, On 2017/03/17 at 23:33:18, Nate Chapin wrote: > This method (1) does enough DocumentLoader-related stuff that I think non-static is better, (2) does more than creating a DocumentWriter, renamed accordingly. Maybe add a comment explaining the scope?
lgtm https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:867: LocalFrame* frame = document->frame(); DCHECK_EQ(frame, m_frame); https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:893: const KURL& overridingURL) { DCHECK_EQ(init.frame(), m_frame);
https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:867: LocalFrame* frame = document->frame(); On 2017/03/21 13:20:37, yhirano wrote: > DCHECK_EQ(frame, m_frame); This is a static helper, so there is no m_frame here. Also, given that one LocalFrame can have many Documents over its lifetime, but one Document is only every associated with one LocalFrame, I'm not sure that DCHECK adds anything meaningful. https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:893: const KURL& overridingURL) { On 2017/03/21 13:20:37, yhirano wrote: > DCHECK_EQ(init.frame(), m_frame); Done. https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/DocumentLoader.h (left): https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.h:219: static DocumentWriter* createWriterFor(const DocumentInit&, On 2017/03/21 09:39:30, Mike West wrote: > On 2017/03/17 at 23:33:18, Nate Chapin wrote: > > This method (1) does enough DocumentLoader-related stuff that I think > non-static is better, (2) does more than creating a DocumentWriter, renamed > accordingly. > > Maybe add a comment explaining the scope? Done.
The CQ bit was checked by japhet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2751833002/#ps80001 (title: "Rebase + nits")
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": 80001, "attempt_start_ts": 1490124426415280,
"parent_rev": "582807cc0713109b51ec8e1adca6d04879a00c18", "commit_rev":
"ace017f26fd8cb8d123238e41c2f0e11f5daacd6"}
Message was sent while issue was closed.
Description was changed from ========== Clean up DocumentWriter creation's FrameLoader interaction There are multiple methods in FrameLoader that take part in the commit and Document creation sequence, with less-than-clear purposes. Also, these methods make little-to-no use of FrameLoader's members, instead primarily interacting with Document, LocalFrameClient, and the DocumentLoader that is driving the commit. Move a bunch of this logic to DocumentLoader, since it's all tied to the loading of a specific Document anyway. BUG= ========== to ========== Clean up DocumentWriter creation's FrameLoader interaction There are multiple methods in FrameLoader that take part in the commit and Document creation sequence, with less-than-clear purposes. Also, these methods make little-to-no use of FrameLoader's members, instead primarily interacting with Document, LocalFrameClient, and the DocumentLoader that is driving the commit. Move a bunch of this logic to DocumentLoader, since it's all tied to the loading of a specific Document anyway. BUG= Review-Url: https://codereview.chromium.org/2751833002 Cr-Commit-Position: refs/heads/master@{#458549} Committed: https://chromium.googlesource.com/chromium/src/+/ace017f26fd8cb8d123238e41c2f... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/ace017f26fd8cb8d123238e41c2f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
