New issue
Advanced search Search tips

Issue 874544 link

Starred by 1 user
Status: Fixed
Owner:
lukasza@chromium.org
Closed: Aug 29
Cc:
kinuko@chromium.org
dcheng@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Let's try to clean-up WebLocalFrame::CommitDataNavigation and related code

Project Member Reported by lukasza@chromium.org, Aug 15 
This bug is a follow-up for https://chromium-review.googlesource.com/c/chromium/src/+/1153564#message-c8c89d1eb5060bb0e77c4a20fe31d424aaaee7e5 which observes that it's kind of awkward that:

- WebLocalFrame::CommitDataNavigation is used for committing normal data navigations and is also used for error navigations

- There are two ways of committing error navigations and one of them is used by exactly one caller (the xss auditor)

- There is considerable duplication of code

Comment 1 by lukasza@chromium.org, Aug 15 

One possible step forward is here: WIP CL @ https://crrev.com/c/1176230
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 29 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/80fea269cc5d175e2be98460f76f2084b3ec5a0b

commit 80fea269cc5d175e2be98460f76f2084b3ec5a0b
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Wed Aug 29 18:31:49 2018

[refactoring] Extract WebLocalFrame::CommitDataNavigationWithRequest.

This CL extracts the second half of CommitDataNavigation method into a
new CommitDataNavigationWithRequest method.  The new method is called by
the old WebLocalFrameImpl::CommitDataNavigation method and by
RenderFrameImpl::LoadNavigationErrorPageInternal.

The CL also consolidates and deduplicates RenderFrameImpl code related
to committing of error pages:

  - It avoids duplication of the code that calculates
    blink::WebFrameLoadType and blink::WebHistoryItem based
    on content::HistoryEntry*

  - It removes the need to pass arguments that either 1) do not differ
    among callers or 2) can be deduced from other arguments.  For example:
    - GURL(kUnreachableWebDataURL)
    - is_client_redirect

This CL should be a pure refactoring, with no change in behavior.

Bug:  874544 
Change-Id: I0088ad6367407ba70aa28c550ca46d7121dd78eb
Reviewed-on: https://chromium-review.googlesource.com/1176230
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587208}
[modify] https://crrev.com/80fea269cc5d175e2be98460f76f2084b3ec5a0b/content/renderer/dom_serializer_browsertest.cc
[modify] https://crrev.com/80fea269cc5d175e2be98460f76f2084b3ec5a0b/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/80fea269cc5d175e2be98460f76f2084b3ec5a0b/content/renderer/render_frame_impl.h
[modify] https://crrev.com/80fea269cc5d175e2be98460f76f2084b3ec5a0b/third_party/blink/public/web/web_local_frame.h
[modify] https://crrev.com/80fea269cc5d175e2be98460f76f2084b3ec5a0b/third_party/blink/public/web/web_remote_frame_client.h
[modify] https://crrev.com/80fea269cc5d175e2be98460f76f2084b3ec5a0b/third_party/blink/renderer/core/frame/web_local_frame_impl.cc
[modify] https://crrev.com/80fea269cc5d175e2be98460f76f2084b3ec5a0b/third_party/blink/renderer/core/frame/web_local_frame_impl.h

Comment 3 by lukasza@chromium.org, Aug 29

Status: Fixed (was: Assigned)
Let's call it fixed?

Comment 4 by lukasza@chromium.org, Aug 29

Cc: kinuko@chromium.org
►

Sign in to add a comment