1163122 - Optimize BrowserViewController.viewDidLoad

Status: RESOLVED FIXED

(Firefox for iOS :: General, defect)

Description

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

In BrowserViewController.viewDidLoad we create a new tab if no tabs were restored. This tab will initially open with the home screen.

Two things:

Can we create the WKWebView for this tab lazily until the user opens a link?

I think in BVC.animateToolbarsWithOffsets() we call layoutIfNeeded() twice:

        self.view.layoutIfNeeded()
        if animated {
            UIView.animateWithDuration(duration, animations: animation, completion: completion)
        } else {
            animation()
        }

Can the above call to layoutIfNeeded() be moved into the 'if animated' block? I don't think it is needed if no animations are run?

Comment 1

[Stephan Leroux [:sleroux]]

I dug into loading the WKWebView lazily and it seems that there is some high coupling between the non-browser HomePanelController and the way the BVC/TabManager is 'opening' it. In viewWillAppear, we're creating a new tab, which by definition is a Browser and requires a WKWebView to parse the inputted URL to determine if we should show the Home screen or not. It looks like it was designed so that the flow of opening a web page or home page uses the same logic within the TabManager.

One idea is we could define a protocol interface named Tabbable which has definitions for methods a Tab would need (get snapshot, show, select, etc), refactor the Browser class to implement that interface, and create a separate, non-browser-based class called NativePanel or something that also implements the Tabbable protocol. We would also move all of the web-specific code into the Browser class from the TabManager. This would give us the following advantages:

1. Able to implement more native panel views without overloading the tab manager with panel specifics
2. Thin down the TabManager to be responsible for only the management of tabs and move browser specific logic to the Browser class
3. Increase startup performance by eliminating the need for a WKWebView load when the user is brought to a fresh instance of the app with no tabs and only the HomePanelViewController.

On top of that, we would need to built our a thin router object that would handle determining what to show for a given tab when the BVC makes a request.

Anyways, just my 2 cents on it. I think this might be more work than we originally thought.

Comment 2

[Stephan Leroux [:sleroux]]

After removing the layoutIfNeeded call and other minor adjustments, it looks like viewDidLoad is more performant now and only contributing to 4.1% (33ms) of the startup delay. The two larger culprits now are:

1. 18.1 % (145ms) BrowserViewControler.viewWillAppear -> TabManager.addTab -> Browser.createWebView
2. 16.0% (121ms) TabManager.init -> WKProcessPool.init

As mentioned above, the current architecture of relying on a WKWebView for routing URLs and the high coupling between a 'Tab' and WebView makes it difficult to fix this without larger refactoring. Additionally, I found out that we do this same thing on Android to facilitate addons loading custom panels which is something we might want eventually. Not sure how to move forward with this for v1

The second point is related to the cost of create a WKProcessPool. Not sure if this is something we want to allocate on a background thread or not.

Should I close this bug and open new ones?

Comment 3

[Richard Newman [:rnewman]]

(In reply to Stephan Leroux [:sleroux] from comment #2)

> Additionally, I found out
> that we do this same thing on Android to facilitate addons loading custom
> panels which is something we might want eventually.

Even on Android we deliberately make home panel add-ons write their data into a Java-visible DB so we can show the home panel without the add-on being loaded.

Perf now trumps possible add-on home panels later.


> Not sure how to move forward with this for v1

Sounds like we need to delay WK stuff until after initial startup, presumably by rolling on with your Tabbable/Tab protocol suggestion. My feeling is that the perf impact is enough that we should do it now.


> Should I close this bug and open new ones?

Sounds like you have some fixes for this one, so let's get those in -- even small wins are wins.

Then yes, another bug for delaying WK init.

Comment 4

[Stephan Leroux [:sleroux]]

Okay sounds good. The fixes I mentioned are already in master so I'll go ahead and close this and open a new bug for delaying WK stuff.