Closed Bug 764971 Opened 12 years ago Closed 11 years ago

Do not show the throbber and the "Connecting..." text for chrome pages

Categories

(Firefox :: Tabbed Browser, defect)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 22

People

(Reporter: marco, Assigned: dao)

References

Details

(Whiteboard: [Snappy:p2])

Attachments

(1 file, 3 obsolete files)

patch v2
2.78 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review 
There's bug 716108, but we should do this for every in-content page (especially now that we are moving a lot of things to in-content pages).
Whiteboard: [Snappy] → [Snappy:p2]
Attached patch v1 (obsolete) — — Splinter Review 
> -              if (!this.mBlank) {
> +              if (!this.mBlank || this.mTabBrowser._isInContentPage(aLocation.spec)) {
>                  this._callProgressListeners("onLocationChange",
>                                              [aWebProgress, aRequest, aLocation,
>                                               aFlags]);
>                }

Need to add the _isInContentPage() condition to trigger the onLocationChange listener so we can show or hide browser chrome based on the in-content page whitelist in XULBrowserWindow.onLocationChange listener.  http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4349
Assignee: nobody → raymond
Attachment #715126 - Flags: review?(gavin.sharp)
Attachment #715126 - Attachment is patch: true
Comment on attachment 715126 [details] [diff] [review]
v1

>       <method name="mTabProgressListener">
>         <parameter name="aTab"/>
>         <parameter name="aBrowser"/>
>-        <parameter name="aStartsBlank"/>
>+        <parameter name="aDontShowThrobberForFirstLoad"/>

>-            mBlank: aStartsBlank,
>+            mBlank: aDontShowThrobberForFirstLoad,

>-              if (!this.mBlank) {
>+              if (!this.mBlank || this.mTabBrowser._isInContentPage(aLocation.spec)) {

>-            var tabListener = this.mTabProgressListener(t, b, uriIsBlankPage);
>+            var tabListener = this.mTabProgressListener(t, b, uriIsBlankPage || uriIsInContentPage);

Setting mBlank based on a "don't show throbber for first load" argument such that extra checks are needed because mBlank can't be trusted anymore seems exceedingly weird.

>+      <method name="_isInContentPage">
>+        <parameter name="aURL"/>
>+        <body>
>+          <![CDATA[
>+            return gInContentPages.some(function(aLocation) {
>+              return aLocation == aURL;
>+            });
>+          ]]>
>+        </body>
>+      </method>

Why doesn't this just check whether this is an about: URI? What's the point of the gInContentPages array?
Attachment #715126 - Flags: review?(gavin.sharp) → review-
Summary: Do not show the throbber and the "Connecting..." text for in-content pages → Do not show the throbber and the "Connecting..." text for chrome pages
(In reply to Dão Gottwald [:dao] from comment #2)
> Why doesn't this just check whether this is an about: URI? What's the point
> of the gInContentPages array?

Some about: pages can be remote resources (e.g. about:credits), we probably want to continue to show the throbber for those.
Maintaining a whitelist seems annoying, but I guess we already have inContentWhitelist. We could in theory introduce a check for the about: URI's target URI scheme being "chrome", but that's probably overkill.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #4)
> Maintaining a whitelist seems annoying, but I guess we already have
> inContentWhitelist.

It's only a mostly random subset of our about: URIs -- one that doesn't necessarily exclude remote pages.

> We could in theory introduce a check for the about:
> URI's target URI scheme being "chrome"

That's what I was just going to suggest.
(In reply to Dão Gottwald [:dao] from comment #5)
> It's only a mostly random subset of our about: URIs -- one that doesn't
> necessarily exclude remote pages.

The ones that we treat as "in-content UI". I can't foresee us actually adding a remote page to this list.
(and in any case the mapping doesn't necessarily need to be perfect)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #5)
> > It's only a mostly random subset of our about: URIs -- one that doesn't
> > necessarily exclude remote pages.
> 
> The ones that we treat as "in-content UI".

Which is only a subset of about: pages that users see.

> I can't foresee us actually adding a remote page to this list.

I can imagine us doing that for services such as sync or the health report.

> (and in any case the mapping doesn't necessarily need to be perfect)

Why would we prefer something imperfect? Is the inner URI not easily accessible when we'd need to check it?
You're assuming that this behavior should be tied to "pages loaded from local resources" rather than "pages we treat as "in-content" UI". I don't really see why that would necessarily be the case.
"in-content" means that 1) we load it in the content area (true for all about/chrome pages tabbrowser gets to see) and 2) we hide the navigation toolbar. How's the latter distinction related to whether we should show a throbber in the tab bar?
That we have some about pages where we hide the navigation toolbar and some where we don't implies a distinction; I don't know what that's meant to be exactly, but given the current list I assume it's related to whether the feature is user-exposed. Assuming we only care about the throbber behavior for those about: pages, we don't need to over-engineer a solution that applies to all about: pages. We already have a simple whitelist that defines the pages we care about.

I'm not particularly concerned about the user impact of the throbber appearing when about:config is loaded, for example.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #11)
> That we have some about pages where we hide the navigation toolbar and some
> where we don't implies a distinction; I don't know what that's meant to be
> exactly, but given the current list I assume it's related to whether the
> feature is user-exposed.

No, it's not that. For instance, about:home, about:privatebrowsing, about:sessionrestore and about:support are not in that list.

> Assuming we only care about the throbber behavior
> for those about: pages, we don't need to over-engineer a solution that
> applies to all about: pages. We already have a simple whitelist that defines
> the pages we care about.

It's still unclear to me why you consider a solution for all about: pages over-engineering. Loosing the dependency on a merely tangentially related list of pages seems to me like it should be a simplification.
(In reply to Dão Gottwald [:dao] from comment #12)
> No, it's not that. For instance, about:home, about:privatebrowsing,
> about:sessionrestore and about:support are not in that list.

Well, it's not *just* that. There are secondary factors certainly. It wouldn't make sense to hide chrome for about:home, privatebrowsing is just informative, etc. But in any case I don't think we ended up at the current state deliberately.

> It's still unclear to me why you consider a solution for all about: pages
> over-engineering.

It's more complicated code for no clear user benefit. But we're getting carried away with an abstract discussion, so let's focus on proposed patches instead.
Attached patch v2 (obsolete) — — Splinter Review 
Added another parameter to mTabProgressListener method so it wouldn't affect the mBlank variable.
Attachment #715126 - Attachment is obsolete: true
Attachment #715457 - Flags: review?(dao)
Comment on attachment 715457 [details] [diff] [review]
v2

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #13)
> (In reply to Dão Gottwald [:dao] from comment #12)
> > It's still unclear to me why you consider a solution for all about: pages
> > over-engineering.
> 
> It's more complicated code

I don't know why you keep saying this. It shouldn't be more code, might well be less, and as I said getting rid of the external dependency on inContentWhitelist (which, as you seem to say, we didn't build deliberately?) should make this code more straightforward.

> for no clear user benefit.

The user benefit is that pages such as about:home, about:privatebrowsing and about:support load or appear to load more snappily. If this doesn't count as a user benefit, the maybe we should wontfix this bug?

> But we're getting
> carried away with an abstract discussion, so let's focus on proposed patches
> instead.

I'll propose a patch.
Attachment #715457 - Flags: review?(dao) → review-
Attached patch patch (obsolete) — — Splinter Review 
Assignee: raymond → dao

        Attachment #715457 -
        Attachment is obsolete: true
Status: NEW → ASSIGNED

        Attachment #715501 -
        Flags: review?(gavin.sharp)

    
    

    
  
What about hiding the "Connecting..." text for every page with "file" as scheme?

    
    

    
  
(In reply to Marco Castelluccio [:marco] from comment #17)
> What about hiding the "Connecting..." text for every page with "file" as
> scheme?

"Connecting..." doesn't just mean connecting, it's also strangely overloaded to mean loading, and file: resources aren't guaranteed to load immediately.
We can do this for built-in resources because we control them.

    
    

    
  
Comment on attachment 715501 [details] [diff] [review]
patch

Doesn't want that check need to be for "jar" rather than "file"? At least that's what e.g. NetUtil.newChannel("about:addons").URI.scheme returns for me. Though in theory an about: URI could point to a remote jar: URI, and so maybe you want to actually check that ((URI instanceof Ci.nsINestedURI) ? URI.innermostURI : URI).scheme is "file", or something like that. Of course even file:// URIs can be "remote" (e.g. on a network drive), and so even that check isn't quite accurate. Dealing with this kind of ambiguity is why I favoured a simple whitelist approach.

Should we check _shouldShowProgress instead of !this.mBlank in onLocationChange on onStatusChange? Seems like we should for onStatusChange, but I'm not sure why we check !this.mBlank before calling onLocationChange.

        Attachment #715501 -
        Flags: review?(gavin.sharp) → review-

    
    

    
  
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #19)
> Comment on attachment 715501 [details] [diff] [review]
> patch
> 
> Doesn't want that check need to be for "jar" rather than "file"?

I had omni.ja disabled when I tested this. So we need both, unless we use innermostURI like you suggest.

> Of course
> even file:// URIs can be "remote" (e.g. on a network drive), and so even
> that check isn't quite accurate. Dealing with this kind of ambiguity is why
> I favoured a simple whitelist approach.

We can choose not to care about Firefox running from a network drive. For the whitelist we would have had to make the same choice.

> Should we check _shouldShowProgress instead of !this.mBlank in
> onLocationChange on onStatusChange? Seems like we should for onStatusChange,
> but I'm not sure why we check !this.mBlank before calling onLocationChange.

The onLocationChange seems unrelated to progress indicators and onStatusChange I think we should leave unchanged, as the status message can be valuable when a chrome page loads remote stuff (e.g. the Get Add-ons iframe in about:addons).

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v2
        — 
          — Splinter Review
      

    


  
about: pointing to jar:http:... seems like an edge case not worth handling, especially since network.jar.open-unsafe-types defaults to false. So I've picked the simpler option of checking for jar: and file:, ignoring innermostURI.

        Attachment #715501 -
        Attachment is obsolete: true

        Attachment #717043 -
        Flags: review?(gavin.sharp)

    
    

    
  
(In reply to Dão Gottwald [:dao] from comment #20)
> We can choose not to care about Firefox running from a network drive. For
> the whitelist we would have had to make the same choice.

I didn't mean Firefox installed on a network drive, I meant loading normal files from file://. But the "about" check makes that irrelevant.

    
  
Blocks: 757455

    
  

        Attachment #717043 -
        Flags: review?(ttaubert)

    
    

    
  
Comment on attachment 717043 [details] [diff] [review]
patch v2

Review of attachment 717043 [details] [diff] [review]:
-----------------------------------------------------------------

I like it.

        Attachment #717043 -
        Flags: review?(ttaubert) → review+

        Attachment #717043 -
        Flags: review?(gavin.sharp)

    
  
Blocks: 855273

    
  
No longer blocks: 757455

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/61b8a5101c5b
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: