Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 2751833002: Clean up DocumentWriter creation's FrameLoader interaction (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months, 1 week ago by Nate Chapin
Modified:
5 months ago
Reviewers:
Mike West, yhirano
CC:
blink-reviews, chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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)
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2751833002/60001
5 months ago (2017-03-17 21:15:08 UTC) #18
commit-bot: I haz the power
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the ...
5 months ago (2017-03-17 21:15:10 UTC) #20
Nate Chapin
DocumentLoader/FrameLoader cleanup! https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Source/core/loader/DocumentLoader.cpp File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Source/core/loader/DocumentLoader.cpp#newcode919 third_party/WebKit/Source/core/loader/DocumentLoader.cpp:919: frameLoader().dispatchDidClearDocumentOfWindowObject(); This single caller appears to meet ...
5 months ago (2017-03-17 23:33:18 UTC) #29
Mike West
Sorry for the delayed response. I think this looks like a pretty reasonable refactoring, and ...
5 months ago (2017-03-21 09:39:30 UTC) #32
yhirano
lgtm https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Source/core/loader/DocumentLoader.cpp File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Source/core/loader/DocumentLoader.cpp#newcode867 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/Source/core/loader/DocumentLoader.cpp#newcode893 third_party/WebKit/Source/core/loader/DocumentLoader.cpp:893: ...
5 months ago (2017-03-21 13:20:37 UTC) #33
Nate Chapin
https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Source/core/loader/DocumentLoader.cpp File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2751833002/diff/60001/third_party/WebKit/Source/core/loader/DocumentLoader.cpp#newcode867 third_party/WebKit/Source/core/loader/DocumentLoader.cpp:867: LocalFrame* frame = document->frame(); On 2017/03/21 13:20:37, yhirano wrote: ...
5 months ago (2017-03-21 19:26:58 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2751833002/80001
5 months ago (2017-03-21 19:27:42 UTC) #37
commit-bot: I haz the power
5 months ago (2017-03-21 21:18:36 UTC) #40
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/ace017f26fd8cb8d123238e41c2f...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b