Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2871133002:
PlzNavigate: make MHTML iframe working (followup) (Closed)
|
Created:
3 years, 7 months ago by arthursonzogni Modified:
3 years, 7 months ago CC:
chromium-reviews, tyoshino+watch_chromium.org, loading-reviews_chromium.org, gavinp+loader_chromium.org, blink-reviews, loading-reviews+fetch_chromium.org, kinuko+watch, Nate Chapin, carlosk, jianli, nasko Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: make MHTML iframe working (followup)
This is a followup to https://crrev.com/2834013002. It doesn't change
the behavior of chrome but refactors some parts.
BUG=705508
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel
Review-Url: https://codereview.chromium.org/2871133002
Cr-Commit-Position: refs/heads/master@{#471722}
Committed: https://chromium.googlesource.com/chromium/src/+/f723e76be72fbbf475f4f0017a2191d986315428
Patch Set 1 : @pfeldman suggestions #Patch Set 2 : @japhet suggestions #
Total comments: 2
Patch Set 3 : Nit #
Messages
Total messages: 28 (22 generated)
The CQ bit was checked by arthursonzogni@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...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:60001) has been deleted
The CQ bit was checked by arthursonzogni@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
arthursonzogni@chromium.org changed reviewers: + japhet@chromium.org, pfeldman@chromium.org
Hi pfeldman@ and japhet@, This is a followup to the suggestions you made in https://crrev.com/2834013002. Please take a look.
Deferring to japhet@ who had more comments. Mine was minor and now looks good.
lgtm https://codereview.chromium.org/2871133002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/LocalFrameClientImpl.cpp (right): https://codereview.chromium.org/2871133002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/LocalFrameClientImpl.cpp:141: local_frame->Loader().GetDocumentLoader()->Fetcher()->Archive(); Nit: you can get the fetcher a little more concisely with local_frame->GetDocument()->Fetcher()->Archive()
The CQ bit was checked by arthursonzogni@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: This issue passed the CQ dry run.
Thanks for the reviews! https://codereview.chromium.org/2871133002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/LocalFrameClientImpl.cpp (right): https://codereview.chromium.org/2871133002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/LocalFrameClientImpl.cpp:141: local_frame->Loader().GetDocumentLoader()->Fetcher()->Archive(); On 2017/05/12 19:30:08, Nate Chapin wrote: > Nit: you can get the fetcher a little more concisely with > local_frame->GetDocument()->Fetcher()->Archive() Done.
The CQ bit was checked by arthursonzogni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org Link to the patchset: https://codereview.chromium.org/2871133002/#ps100001 (title: "Nit")
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": 100001, "attempt_start_ts": 1494845896264880,
"parent_rev": "fb99e7befdcc90189d48926d63c04fbf68618f16", "commit_rev":
"f723e76be72fbbf475f4f0017a2191d986315428"}
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: make MHTML iframe working (followup) This is a followup to https://crrev.com/2834013002. It doesn't change the behavior of chrome but refactors some parts. BUG=705508 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ========== to ========== PlzNavigate: make MHTML iframe working (followup) This is a followup to https://crrev.com/2834013002. It doesn't change the behavior of chrome but refactors some parts. BUG=705508 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel Review-Url: https://codereview.chromium.org/2871133002 Cr-Commit-Position: refs/heads/master@{#471722} Committed: https://chromium.googlesource.com/chromium/src/+/f723e76be72fbbf475f4f0017a21... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as https://chromium.googlesource.com/chromium/src/+/f723e76be72fbbf475f4f0017a21... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
