Issue 907527 link

Starred by 1 user

Issue metadata

Status:	Available
Owner:	djean@chromium.org
Cc:	marq@chromium.org edchin@chromium.org rohitrao@chromium.org ios-bugs@chromium.org
Components:	Internals
EstimatedDays:	----
NextAction:	----
OS:	iOS
Pri:	3
Type:	Bug

Merge all the ways of opening a new tab

Project Member Reported by gambard@chromium.org, Nov 21

Issue description

There are at least 3 different ways of opening a URL:
- URLLoader -loadURLWithParams:
- URLLoader -webPageOrderedOpen:
- ApplicationCommands -openURLInNewTab:

Maybe some of them should be merged.

Comment 1 by djean@chromium.org, Jan 15

Cc: edchin@chromium.org marq@chromium.org rohitrao@chromium.org
Owner: djean@chromium.org

From looking at this I would add NavigationManager::LoadURLWithParams as another way to open a new tab, even though all 3 path above end up calling it, we are also calling it directly in a few locations.  Here's a summary of all call site for opening a Url I could find, some are using one of the four, some are using 2, 3 or 4 of them.

A: NavigationManager::LoadURLWithParams (NavigationManagerImpl)
B: CommandDispatcher::openURLInNewTab (MC)
C: UrlLoader::webPageOrderedOpen (BVC)
D: UrlLoader::loadURLWithParams (BVC)

 A B C   ios/chrome/app/main_controller.mm
   B     ios/chrome/app/application_delegate/app_state.mm
 A       ios/chrome/browser/prerender/preload_controller.mm
 A       ios/chrome/browser/reading_list/reading_list_distiller_page.mm
 A       ios/chrome/browser/tabs/tab_model.mm
       D ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm
     C D ios/chrome/browser/ui/bookmarks/bookmark_interaction_controller.mm
 A B C D ios/chrome/browser/ui/browser_view_controller.mm
     C D ios/chrome/browser/ui/content_suggestions/ntp_home_mediator.mm
   B     ios/chrome/browser/ui/content_suggestions/content_suggestions_coordinator.mm
     C   ios/chrome/browser/ui/history/history_clear_browsing_data_coordinator.mm
     C D ios/chrome/browser/ui/history/history_table_view_controller.mm
   B     ios/chrome/browser/ui/key_commands_provider.mm
       D ios/chrome/browser/ui/location_bar/location_bar_coordinator.mm
       D ios/chrome/browser/ui/ntp/incognito_view.mm
       D ios/chrome/browser/ui/omnibox/popup/shortcuts/shortcuts_mediator.mm
     C   ios/chrome/browser/ui/page_info/page_info_legacy_coordinator.mm
   B     ios/chrome/browser/ui/popup_menu/popup_menu_action_handler.mm
     C D ios/chrome/browser/ui/reading_list/reading_list_coordinator.mm
     C   ios/chrome/browser/ui/recent_tabs/recent_tabs_table_view_controller.mm
   B     ios/chrome/browser/ui/sad_tab/sad_tab_coordinator.mm
   B     ios/chrome/browser/ui/sad_tab/sad_tab_legacy_coordinator.mm
       D ios/chrome/browser/ui/static_content/static_html_view_controller.mm
 A       ios/chrome/browser/ui/tab_grid/tab_grid_mediator.mm
 A       ios/chrome/browser/ui/tab_grid/tab_grid_url_loader.mm
 A B     ios/chrome/browser/ui/tabs/tab_strip_controller.mm
   B     ios/chrome/browser/upgrade/upgrade_center.mm
 A       ios/chrome/browser/url_loading/url_loading_util.mm
 A       ios/web_view/internal/cwv_web_view.mm
 A       ios/web/navigation/navigation_manager_impl.mm
 A       ios/web/navigation/wk_based_navigation_manager_impl.mm
 A       ios/web/shell/view_controller.mm
 A       ios/web/web_state/ui/crw_web_controller.mm

(table exclude test related files)

Comment 2 by djean@chromium.org, Jan 15

Something that should also be part of the list is BVC::openNewTabInCurrentMode (called by BVC<UrlLoader>::webPageOrderedOpen) because it is also called two more times internally, to the BVC, for:
- (void)showHelpPage
- (void)webState:(web::WebState*)webState
    handleContextMenu:(const web::ContextMenuParams&)params {
  when isImage + ACTION_OPEN_IMAGE_IN_NEW_TAB

Comment 3 by djean@chromium.org, Jan 15

There's another path, WebState::OpenURL -> WebStateDelegateBridge::OpenURLFromWebState -> BVC::webState:openURLWithParams -> TabModel::insertTabWithURL | NavigationManager::LoadURLWithParams.

Call sites for WebState::OpenURL are

ios/chrome/browser/infobars/infobar_manager_impl.mm
ios/chrome/browser/interstitials/ios_chrome_controller_client.mm
ios/chrome/browser/reading_list/reading_list_web_state_observer.mm
ios/chrome/browser/web/blocked_popup_tab_helper.mm

Updated tables is:

A: NavigationManager::LoadURLWithParams (NavigationManagerImpl)
B: CommandDispatcher::openURLInNewTab (MC)
C: UrlLoader::webPageOrderedOpen (BVC)
D: UrlLoader::loadURLWithParams (BVC)
E: BVC::openNewTabInCurrentMode
F: WebState::OpenURL

 A B C       ios/chrome/app/main_controller.mm
   B         ios/chrome/app/application_delegate/app_state.mm
           F ios/chrome/browser/infobars/infobar_manager_impl.mm
           F ios/chrome/browser/interstitials/ios_chrome_controller_client.mm
 A           ios/chrome/browser/prerender/preload_controller.mm
 A           ios/chrome/browser/reading_list/reading_list_distiller_page.mm
           F ios/chrome/browser/reading_list/reading_list_web_state_observer.mm
 A           ios/chrome/browser/tabs/tab_model.mm
       D     ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm
     C D     ios/chrome/browser/ui/bookmarks/bookmark_interaction_controller.mm
 A B C D E   ios/chrome/browser/ui/browser_view_controller.mm
     C D     ios/chrome/browser/ui/content_suggestions/ntp_home_mediator.mm
   B         ios/chrome/browser/ui/content_suggestions/content_suggestions_coordinator.mm
     C       ios/chrome/browser/ui/history/history_clear_browsing_data_coordinator.mm
     C D     ios/chrome/browser/ui/history/history_table_view_controller.mm
   B         ios/chrome/browser/ui/key_commands_provider.mm
       D     ios/chrome/browser/ui/location_bar/location_bar_coordinator.mm
       D     ios/chrome/browser/ui/ntp/incognito_view.mm
       D     ios/chrome/browser/ui/omnibox/popup/shortcuts/shortcuts_mediator.mm
     C       ios/chrome/browser/ui/page_info/page_info_legacy_coordinator.mm
   B         ios/chrome/browser/ui/popup_menu/popup_menu_action_handler.mm
     C D     ios/chrome/browser/ui/reading_list/reading_list_coordinator.mm
     C       ios/chrome/browser/ui/recent_tabs/recent_tabs_table_view_controller.mm
   B         ios/chrome/browser/ui/sad_tab/sad_tab_coordinator.mm
   B         ios/chrome/browser/ui/sad_tab/sad_tab_legacy_coordinator.mm
       D     ios/chrome/browser/ui/static_content/static_html_view_controller.mm
 A           ios/chrome/browser/ui/tab_grid/tab_grid_mediator.mm
 A           ios/chrome/browser/ui/tab_grid/tab_grid_url_loader.mm
 A B         ios/chrome/browser/ui/tabs/tab_strip_controller.mm
   B         ios/chrome/browser/upgrade/upgrade_center.mm
 A           ios/chrome/browser/url_loading/url_loading_util.mm
           F ios/chrome/browser/web/blocked_popup_tab_helper.mm
 A           ios/web_view/internal/cwv_web_view.mm
 A           ios/web/navigation/navigation_manager_impl.mm
 A           ios/web/navigation/wk_based_navigation_manager_impl.mm
 A           ios/web/shell/view_controller.mm
 A           ios/web/web_state/ui/crw_web_controller.mm


(table exclude test related files)

Comment 4 by eugene...@chromium.org, Jan 15

Components: -UI>Browser>Navigation Internals

Comment 5 by djean@chromium.org, Jan 16 (6 days ago)

Adding TabOpening::dismissModalsAndOpenSelectedTabInMode (implemented in MainController) which is the entry point for application_delegate tab opening.  It's also called in MC by openURLInNewTab (B).


A: NavigationManager::LoadURLWithParams (NavigationManagerImpl)
B: CommandDispatcher::openURLInNewTab (MC)
C: UrlLoader::webPageOrderedOpen (BVC)
D: UrlLoader::loadURLWithParams (BVC)
E: BVC::openNewTabInCurrentMode
F: WebState::OpenURL
G: TabOpening::dismissModalsAndOpenSelectedTabInMode (MC)

 A B C       G ios/chrome/app/main_controller.mm
   B           ios/chrome/app/application_delegate/app_state.mm
             G ios/chrome/app/application_delegate/user_activity_handler.mm
             G ios/chrome/app/application_delegate/url_opener.mm
           F   ios/chrome/browser/infobars/infobar_manager_impl.mm
           F   ios/chrome/browser/interstitials/ios_chrome_controller_client.mm
 A             ios/chrome/browser/prerender/preload_controller.mm
 A             ios/chrome/browser/reading_list/reading_list_distiller_page.mm
           F   ios/chrome/browser/reading_list/reading_list_web_state_observer.mm
 A             ios/chrome/browser/tabs/tab_model.mm
       D       ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm
     C D       ios/chrome/browser/ui/bookmarks/bookmark_interaction_controller.mm
 A B C D E     ios/chrome/browser/ui/browser_view_controller.mm
     C D       ios/chrome/browser/ui/content_suggestions/ntp_home_mediator.mm
   B           ios/chrome/browser/ui/content_suggestions/content_suggestions_coordinator.mm
     C         ios/chrome/browser/ui/history/history_clear_browsing_data_coordinator.mm
     C D       ios/chrome/browser/ui/history/history_table_view_controller.mm
   B           ios/chrome/browser/ui/key_commands_provider.mm
       D       ios/chrome/browser/ui/location_bar/location_bar_coordinator.mm
       D       ios/chrome/browser/ui/ntp/incognito_view.mm
       D       ios/chrome/browser/ui/omnibox/popup/shortcuts/shortcuts_mediator.mm
     C         ios/chrome/browser/ui/page_info/page_info_legacy_coordinator.mm
   B           ios/chrome/browser/ui/popup_menu/popup_menu_action_handler.mm
     C D       ios/chrome/browser/ui/reading_list/reading_list_coordinator.mm
     C         ios/chrome/browser/ui/recent_tabs/recent_tabs_table_view_controller.mm
   B           ios/chrome/browser/ui/sad_tab/sad_tab_coordinator.mm
   B           ios/chrome/browser/ui/sad_tab/sad_tab_legacy_coordinator.mm
       D       ios/chrome/browser/ui/static_content/static_html_view_controller.mm
 A             ios/chrome/browser/ui/tab_grid/tab_grid_mediator.mm
 A             ios/chrome/browser/ui/tab_grid/tab_grid_url_loader.mm
 A B           ios/chrome/browser/ui/tabs/tab_strip_controller.mm
   B           ios/chrome/browser/upgrade/upgrade_center.mm
 A             ios/chrome/browser/url_loading/url_loading_util.mm
           F   ios/chrome/browser/web/blocked_popup_tab_helper.mm
 A             ios/web_view/internal/cwv_web_view.mm
 A             ios/web/navigation/navigation_manager_impl.mm
 A             ios/web/navigation/wk_based_navigation_manager_impl.mm
 A             ios/web/shell/view_controller.mm
 A             ios/web/web_state/ui/crw_web_controller.mm


(table exclude test related files)

Comment 6 by djean@chromium.org, Jan 16 (6 days ago)

What extra work does each method do (or can do) between call site and 
NavigationManager::LoadURLWithParams

Functions tagged with *] are part of the internal tab opening flow, meaning that 
they are called only from function tagged as url opening point of entry (A to G)

F] WebState::OpenURL
 - clear transient content (???)
 - calls WebStateDelegateBridge::OpenURLFromWebState, which
 - calls BVC::webState:openURLWithParams, which
 - calls TabModel::insertTabWithURL or NavigationManager::LoadURLWithParams.

B] MC<CommandDispatcher>::openURLInNewTab
 - If url is valid and fromChrome:
  - dismissModalsAndOpenSelectedTabInMode
 - If url is valid and not fromChrome:
  - dismisses modals (MC::dismissModalDialogsWithCompletion)
  - sets mode (incognito|normal)
  - calls BVC::webPageOrderedOpen.
 - if url is not valid:
  - if switching incognito:
   - updates snapshot of current webstate.
   - switches mode the call CommandDispatcher::openURLInNewTab again.
  - calls feature_engagement::NotifyNewTabEventForCommand.
  - calls BVC::openNewTabFromOriginPoint.

G] MC::dismissModalsAndOpenSelectedTabInMode
  - dismisses modals (MC::dismissModalDialogsWithCompletion)
  - calls MC::openSelectedTabInMode.

*] MC::openSelectedTabInMode
 - handles post opening cleaning tasks.
 - calls one of:
 -  TabModel::insertTabWithLoadParams,
 -  TabSwitcher::dismissWithNewTabAnimationToModel (unfortunate name),
 -  MC::openOrReuseTabInMode
 - displays 'restore tabs' helper.
 - set's webstate navigation item virtual url (pretty printed url for display)

*] MC::openOrReuseTabInMode
 - calls either
 -  TabModel::insertTabWithLoadParams.
 -  NavigationManager::LoadURLWithParams.

D] BVC<UrlLoader>::loadURLWithParams
  - dismisses BookmarkModalController.
  - calls switchToTabWithParams (note that caller, here knows this will happen because they reqested it)
  - notifies OmniboxGeolocationController that locationBarDidSubmitURL.
  - records metrics for RecordActionFromOmnibox.
  - handles induced crash.
  - asks the prerender service to load this URL if it can.
  - calls UrlLoader::webPageOrderedOpen for pages disallowed in incognito.
  - calls LoadTimingTabHelper::DidInitiatePageLoad() to record load start time.
  - calls NavigationManager::LoadURLWithParams.
  - hides the NTP.

C] BVC<UrlLoader>::webPageOrderedOpen
 - notifies feature_engagement::NotifyNewTabEvent.
 - transfers control to MC::openURLInNewTab if request incognito state mismatches BVC incognito state. 
 - calls BVC::openNewTabInCurrentMode.

E] BVC::openNewTabInCurrentMode
 - animates tab opening (exist fullscreen if needed)
 - calls tabModel to insert insertTabWithURL at proper position in the list.

*] BVC::openNewTabFromOriginPoint
 - notes originating tap point.
 - updates snapshot of current webstate (if possible)
 - calls TabModel::insertTabWithLoadParams.
 - computes tab opening metrics.
 - optionaly focuses omnibox.

*] TabModel::insertTabWithURL
 - calls insertTabWithLoadParams.

*] TabGridAdaptor<TabSwitcher>::dismissWithNewTabAnimationToModel
 - prepares transition.
 - calls insertTabWithLoadParams.
 - completes transition.

*] TabModel::insertTabWithLoadParams
 - computes tab insertion place and type.
 - creates WebState. 
 - calls NavigationManager::LoadURLWithParams.
 - returns a Tab for the newly created webstate.

Project Member

Comment 7 by bugdroid1@chromium.org, Yesterday (46 hours ago)

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

commit 4ffade7f71611394e89b8e7d496236de905598b8
Author: David Jean <djean@google.com>
Date: Mon Jan 21 09:13:01 2019

[ios] Moving omnibox geolocation call to location bar coordinator

Chipping at UrlLoader from another direction.
Key difference from original is that the omnibox geolocation controller would, now, not be called
when calls to UrlLoader::loadURLWithParams from elsewhere than the location bar happen,
but following the comments I saw in the code, I'm not sure it is an issue...
Removed unused parameters to locationBarDidSubmitURL, by creating a parameter-less version
and deprecating the other one (temporarily).

Bug: 907527

Change-Id: I32562e23938ac4305c61c8813115219f64bd32fc
Reviewed-on: https://chromium-review.googlesource.com/c/1420838
Commit-Queue: David Jean <djean@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Stepan Khapugin <stkhapugin@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624536}
[modify] https://crrev.com/4ffade7f71611394e89b8e7d496236de905598b8/ios/chrome/browser/geolocation/omnibox_geolocation_controller.h
[modify] https://crrev.com/4ffade7f71611394e89b8e7d496236de905598b8/ios/chrome/browser/geolocation/omnibox_geolocation_controller.mm
[modify] https://crrev.com/4ffade7f71611394e89b8e7d496236de905598b8/ios/chrome/browser/ui/location_bar/BUILD.gn
[modify] https://crrev.com/4ffade7f71611394e89b8e7d496236de905598b8/ios/chrome/browser/ui/location_bar/location_bar_coordinator.mm
[modify] https://crrev.com/4ffade7f71611394e89b8e7d496236de905598b8/ios/chrome/browser/url_loading/BUILD.gn
[modify] https://crrev.com/4ffade7f71611394e89b8e7d496236de905598b8/ios/chrome/browser/url_loading/url_loading_util.mm

Project Member

Comment 8 by bugdroid1@chromium.org, Yesterday (42 hours ago)

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chrome/ios_internal.git/+/ad267e575550a8f4ea90af15acd9f24efa4aac55

commit ad267e575550a8f4ea90af15acd9f24efa4aac55
Author: David Jean <djean@chromium.org>
Date: Mon Jan 21 13:07:44 2019

Project Member

Comment 9 by bugdroid1@chromium.org, Today (20 hours ago)

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

commit e646601e067bab061fe0e0e48c6852d795be946c
Author: David Jean <djean@google.com>
Date: Tue Jan 22 11:26:17 2019

[ios] moving bookmark controller dismissal out of loadURLWithParams

Moving it to CRWWebStateObserver::didStartNavigation observer cb.  I think the only way BookmarkModalController is still up is when it is up and user triggers a load from an external app, like iGSA.

Bug: 907527
Change-Id: I73ad94294ef272a415b6c87a73422dc92695ee2c
Reviewed-on: https://chromium-review.googlesource.com/c/1421184
Reviewed-by: Mark Cogan <marq@chromium.org>
Commit-Queue: David Jean <djean@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624759}
[modify] https://crrev.com/e646601e067bab061fe0e0e48c6852d795be946c/ios/chrome/browser/ui/browser_view_controller.mm

►

Issue 907527 link

Issue metadata

Merge all the ways of opening a new tab

Issue description

Comment 1 by djean@chromium.org, Jan 15

Comment 1 Deleted

Comment 2 by djean@chromium.org, Jan 15

Comment 2 Deleted

Comment 3 by djean@chromium.org, Jan 15

Comment 3 Deleted

Comment 4 by eugene...@chromium.org, Jan 15

Comment 4 Deleted

Comment 5 by djean@chromium.org, Jan 16 (6 days ago)

Comment 5 Deleted

Comment 6 by djean@chromium.org, Jan 16 (6 days ago)

Comment 6 Deleted

Comment 7 by bugdroid1@chromium.org, Yesterday (46 hours ago)

Comment 7 Deleted

Comment 8 by bugdroid1@chromium.org, Yesterday (42 hours ago)

Comment 8 Deleted

Comment 9 by bugdroid1@chromium.org, Today (20 hours ago)

Comment 9 Deleted

Sign in to add a comment