New issue
Advanced search Search tips

Issue 903830 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Task

Blocking:
issue 903325



Sign in to add a comment

Refactor URLLoader protocol into a model-layer object or utility functions.

Project Member Reported by marq@chromium.org, Nov 9

Issue description

As part of removing extraneous responsibilities from the BVC, refactor URLLoader out of it (and other places) and into pure model-layer objects or functions.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 19

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

commit 1dd20acc4de0b99e62a3cf63368ef8beddafc156
Author: Mark Cogan <marq@google.com>
Date: Wed Dec 19 09:08:25 2018

[iOS] Factor loadJavaScriptFromLocationBar: out of URLLoader

This CL factors loadJavaScriptFromLocationBar: into a free utility function, housed in c/b/url_loading.

The utility acts on <GURL, BrowserState, WebState>, so it enacpsulates the logic for extracting JS from the URL (which used to be repeated).

The major ugliness is in injecting the needed WebStateList into the bookmarks code, since the interaction controller and view controller cyclically create each other.

Bug: 903830
Change-Id: I9f42f3b4d1996ec57aa457d6cd9fb9873e099744
Reviewed-on: https://chromium-review.googlesource.com/c/1382133
Reviewed-by: edchin <edchin@chromium.org>
Commit-Queue: Mark Cogan <marq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617774}
[modify] https://crrev.com/1dd20acc4de0b99e62a3cf63368ef8beddafc156/ios/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/1dd20acc4de0b99e62a3cf63368ef8beddafc156/ios/chrome/browser/ui/bookmarks/BUILD.gn
[modify] https://crrev.com/1dd20acc4de0b99e62a3cf63368ef8beddafc156/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h
[modify] https://crrev.com/1dd20acc4de0b99e62a3cf63368ef8beddafc156/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm
[modify] https://crrev.com/1dd20acc4de0b99e62a3cf63368ef8beddafc156/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller_unittest.mm
[modify] https://crrev.com/1dd20acc4de0b99e62a3cf63368ef8beddafc156/ios/chrome/browser/ui/bookmarks/bookmark_interaction_controller.h
[modify] https://crrev.com/1dd20acc4de0b99e62a3cf63368ef8beddafc156/ios/chrome/browser/ui/bookmarks/bookmark_interaction_controller.mm
[modify] https://crrev.com/1dd20acc4de0b99e62a3cf63368ef8beddafc156/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/1dd20acc4de0b99e62a3cf63368ef8beddafc156/ios/chrome/browser/ui/location_bar/BUILD.gn
[modify] https://crrev.com/1dd20acc4de0b99e62a3cf63368ef8beddafc156/ios/chrome/browser/ui/location_bar/location_bar_coordinator.mm
[modify] https://crrev.com/1dd20acc4de0b99e62a3cf63368ef8beddafc156/ios/chrome/browser/ui/tab_grid/tab_grid_url_loader.mm
[modify] https://crrev.com/1dd20acc4de0b99e62a3cf63368ef8beddafc156/ios/chrome/browser/ui/url_loader.h
[add] https://crrev.com/1dd20acc4de0b99e62a3cf63368ef8beddafc156/ios/chrome/browser/url_loading/BUILD.gn
[add] https://crrev.com/1dd20acc4de0b99e62a3cf63368ef8beddafc156/ios/chrome/browser/url_loading/url_loading_util.h
[add] https://crrev.com/1dd20acc4de0b99e62a3cf63368ef8beddafc156/ios/chrome/browser/url_loading/url_loading_util.mm
[modify] https://crrev.com/1dd20acc4de0b99e62a3cf63368ef8beddafc156/ios/chrome/test/fakes/fake_url_loader.mm

Project Member

Comment 2 by bugdroid1@chromium.org, Dec 19

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

commit 443101b750d15a2453f909d245534ee9d5a06068
Author: Mark Cogan <marq@google.com>
Date: Wed Dec 19 18:43:54 2018

[iOS] Factor restoreTabWithSessionID: out of URLLoader.

This CL factors another method out of URLLoader, replacing it with a free function.

The two prior implementors of this method both did the same thing, aside from using different WindowOpenDispositions. This CL makes that value explicit as a parameter; it matters for Recent Tabs, which clobbers the current tab when view from an open tab, but which opens a new tab when view from the tab grid.

This means adding more model-layer specifics to the recent tabs view controller; that will continue until the URLOpener refactor is complete, and then the model logic in Recent tabs can be hoisted into its mediator.

Bug: 903830
Change-Id: I758f16d6b25ccaf36728244ec377495fc26bbacd
Reviewed-on: https://chromium-review.googlesource.com/c/1382435
Commit-Queue: Mark Cogan <marq@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: edchin <edchin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617891}
[modify] https://crrev.com/443101b750d15a2453f909d245534ee9d5a06068/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/443101b750d15a2453f909d245534ee9d5a06068/ios/chrome/browser/ui/recent_tabs/BUILD.gn
[modify] https://crrev.com/443101b750d15a2453f909d245534ee9d5a06068/ios/chrome/browser/ui/recent_tabs/recent_tabs_table_view_controller.h
[modify] https://crrev.com/443101b750d15a2453f909d245534ee9d5a06068/ios/chrome/browser/ui/recent_tabs/recent_tabs_table_view_controller.mm
[modify] https://crrev.com/443101b750d15a2453f909d245534ee9d5a06068/ios/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm
[modify] https://crrev.com/443101b750d15a2453f909d245534ee9d5a06068/ios/chrome/browser/ui/tab_grid/tab_grid_url_loader.mm
[modify] https://crrev.com/443101b750d15a2453f909d245534ee9d5a06068/ios/chrome/browser/ui/url_loader.h
[modify] https://crrev.com/443101b750d15a2453f909d245534ee9d5a06068/ios/chrome/browser/url_loading/BUILD.gn
[modify] https://crrev.com/443101b750d15a2453f909d245534ee9d5a06068/ios/chrome/browser/url_loading/url_loading_util.h
[modify] https://crrev.com/443101b750d15a2453f909d245534ee9d5a06068/ios/chrome/browser/url_loading/url_loading_util.mm
[modify] https://crrev.com/443101b750d15a2453f909d245534ee9d5a06068/ios/chrome/test/fakes/fake_url_loader.mm

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 20

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

commit 93ed21ebaad36246019490ec3736ab14affcc568
Author: Mark Cogan <marq@google.com>
Date: Thu Dec 20 15:04:14 2018

[iOS] Factor loadSessionTab: out of URLLoader

This CL factors loadSessionTab: out of URLLoader.

This was only used by Recent Tabs, so this CL just shifts all of the implementation logic into a local model update. This means injecting a WebStateList into the Recent Tabs VC, but this can be refactored later.


Bug: 903830
Change-Id: I305f3f420f7fb459ad45f89eb0e6824a1215060d
Reviewed-on: https://chromium-review.googlesource.com/c/1382424
Commit-Queue: Mark Cogan <marq@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618212}
[modify] https://crrev.com/93ed21ebaad36246019490ec3736ab14affcc568/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/93ed21ebaad36246019490ec3736ab14affcc568/ios/chrome/browser/ui/recent_tabs/BUILD.gn
[modify] https://crrev.com/93ed21ebaad36246019490ec3736ab14affcc568/ios/chrome/browser/ui/recent_tabs/recent_tabs_table_view_controller.h
[modify] https://crrev.com/93ed21ebaad36246019490ec3736ab14affcc568/ios/chrome/browser/ui/recent_tabs/recent_tabs_table_view_controller.mm
[modify] https://crrev.com/93ed21ebaad36246019490ec3736ab14affcc568/ios/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm
[modify] https://crrev.com/93ed21ebaad36246019490ec3736ab14affcc568/ios/chrome/browser/ui/tab_grid/tab_grid_url_loader.mm
[modify] https://crrev.com/93ed21ebaad36246019490ec3736ab14affcc568/ios/chrome/browser/ui/url_loader.h
[modify] https://crrev.com/93ed21ebaad36246019490ec3736ab14affcc568/ios/chrome/test/fakes/fake_url_loader.mm

Project Member

Comment 4 by bugdroid1@chromium.org, Dec 20

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

commit 53e76c7736924adb26cc5b50320a6ae57bc0009c
Author: Mark Cogan <marq@google.com>
Date: Thu Dec 20 16:12:46 2018

[iOS] Collect URLLoader-related methods in BVC.

This CL does some housekeeping to make further URLLoader refactoring easier.

The BVC animateNewTabInBackgroundFromPoint:withCompletion: method is only used in the URLLoader helper method openNewTabInCurrentMode:, so it is just inlined into that method

switchToTabWithParams: is only called from loadURLWithParams:, so it's moved to be with the other URLLoader helpers.

This should make it easier to start to move the 300+ lines of URLLoader code out of BVC.

Bug: 903830
Change-Id: I11133deeec5dff78243faba9af1796ae6447da45
Reviewed-on: https://chromium-review.googlesource.com/c/1383035
Commit-Queue: Mark Cogan <marq@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618230}
[modify] https://crrev.com/53e76c7736924adb26cc5b50320a6ae57bc0009c/ios/chrome/browser/ui/browser_view_controller.mm

Components: Internals
Project Member

Comment 6 by bugdroid1@chromium.org, Jan 8

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

commit a0ed5ceba77cad2a254fef72bcdba3983bea07dc
Author: Mark Cogan <marq@google.com>
Date: Tue Jan 08 16:43:18 2019

[iOS] pull BVC URL loading logic into into helper function.

This CL continues the process of pulling model-layer URL loading logic out of the URLLoader protocol (and thus out of the BVC).

Here the bulk of the logic in -loadURLWithParams: is shifted into a LoadURL() helper function. To handle the various cases where further action is needed after calling this, a URLLoadResult enum is added.

The flag for tracking when a pageload is from a prerender is shifted into the prerender service instead of the BVC.

There are no unit tests on url_loading_util. There probably should be.

Bug: 903830
Change-Id: Ie5caab470439bcbf4768c5226bd7e5d37af784de
Reviewed-on: https://chromium-review.googlesource.com/c/1398201
Commit-Queue: Mark Cogan <marq@chromium.org>
Reviewed-by: David Jean <djean@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620754}
[modify] https://crrev.com/a0ed5ceba77cad2a254fef72bcdba3983bea07dc/ios/chrome/browser/prerender/prerender_service.h
[modify] https://crrev.com/a0ed5ceba77cad2a254fef72bcdba3983bea07dc/ios/chrome/browser/prerender/prerender_service.mm
[modify] https://crrev.com/a0ed5ceba77cad2a254fef72bcdba3983bea07dc/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/a0ed5ceba77cad2a254fef72bcdba3983bea07dc/ios/chrome/browser/url_loading/BUILD.gn
[modify] https://crrev.com/a0ed5ceba77cad2a254fef72bcdba3983bea07dc/ios/chrome/browser/url_loading/url_loading_util.h
[modify] https://crrev.com/a0ed5ceba77cad2a254fef72bcdba3983bea07dc/ios/chrome/browser/url_loading/url_loading_util.mm

Sign in to add a comment