517755 - use smart getters in View Source window

Status: RESOLVED FIXED

(Toolkit :: View Source, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

just like browser.js

Comment 1

[[e-mail]]

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

>diff --git a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_viewsource.js b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_viewsource.js
[I'm not a browser peer]

>+    gBrowser.docShell
>+            .QueryInterface(Ci.nsIInterfaceRequestor)
>+            .getInterface(Ci.nsISelectionDisplay)
>+            .QueryInterface(Ci.nsISelectionController)
>+            .scrollSelectionIntoView(Ci.nsISelectionController.SELECTION_NORMAL,
>+                                     Ci.nsISelectionController.SELECTION_ANCHOR_REGION,
>+                                     true);
Could this use getSelectionController() ?

>+function getNavToolbox() document.getElementById("appcontent");
This is not my idea of readability... do we have precedent for this?

Comment 2

[Dão Gottwald [:dao]]

Attachment: patch v2

(In reply to comment #1)
> (From update of attachment 401699 [details] [diff] [review])
> >diff --git a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_viewsource.js b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_viewsource.js
> [I'm not a browser peer]

Yeah, feel free to ignore that change.

> >+    gBrowser.docShell
> >+            .QueryInterface(Ci.nsIInterfaceRequestor)
> >+            .getInterface(Ci.nsISelectionDisplay)
> >+            .QueryInterface(Ci.nsISelectionController)
> >+            .scrollSelectionIntoView(Ci.nsISelectionController.SELECTION_NORMAL,
> >+                                     Ci.nsISelectionController.SELECTION_ANCHOR_REGION,
> >+                                     true);
> Could this use getSelectionController() ?

yep

> >+function getNavToolbox() document.getElementById("appcontent");
> This is not my idea of readability... do we have precedent for this?

Not in toolkit, afaik.

I've also added a getter for the string bundle.

Comment 3

[[e-mail]]

(In reply to comment #2)
> (In reply to comment #1)
> > >+function getNavToolbox() document.getElementById("appcontent");
> > This is not my idea of readability... do we have precedent for this?
> Not in toolkit, afaik.
You then removed that particular one only, but unfortunately I had just picked on that one as being easiest to find in what Bugzilla thinks is a reasonably sized textarea on the machine I was using to review.

Comment 4

[[e-mail]]

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

>+  var webnav = getWebNavigation();
>+  if (webnav) {
>+    delete this.gPageLoader;
>+    return this.gPageLoader = webnav.QueryInterface(Ci.nsIWebPageDescriptor);
>+  }
>+  return null;
Nit: inconsistent with the code above which uses if (!foo) return null;

>+    gBrowser.addProgressListener(gViewSourceProgressListener,
>+                                 Ci.nsIWebProgress.NOTIFY_ALL);
Nit: this is a lie, it's only interested in location changes.

>+function isHistoryEnabled()
>+  !gBrowser.hasAttribute("disablehistory");
Ah, now this was the ugliest one, but not forgetting...

>-function getPPBrowser()
>-{
>-  return document.getElementById("content");
>-}
>+function getPPBrowser() gBrowser;
> 
>+// printUtils.js uses this
... printUtils uses getPPBrowser() too ;-)

>+  var viewSourceBundle = gViewSourceBundle;
This seems somewhat pointless?

>+  selCon.setDisplaySelection(selCon.SELECTION_ON);
I know why this works, but I'd still prefer Ci.nsISelectionController

Comment 5

[Dão Gottwald [:dao]]

(In reply to comment #4)
> >+function isHistoryEnabled()
> >+  !gBrowser.hasAttribute("disablehistory");
> Ah, now this was the ugliest one, but not forgetting...

I'm not sure I see your point. Are you saying that we shouldn't use that language feature at all? Maybe it doesn't immediately match your sense of readability just because it's a fairly new feature?

> >+function getPPBrowser() gBrowser;
> > 
> >+// printUtils.js uses this
> ... printUtils uses getPPBrowser() too ;-)

Yeah, but it has PP in its name, which makes that somewhat more discoverable.

> >+  selCon.setDisplaySelection(selCon.SELECTION_ON);
> I know why this works, but I'd still prefer Ci.nsISelectionController

I prefer it this way (less XPCOM foo), but ok.

Comment 6

[Dão Gottwald [:dao]]

Attachment: patch v3

Comment 7

[[e-mail]]

(In reply to comment #5)
> (In reply to comment #4)
> > >+function isHistoryEnabled()
> > >+  !gBrowser.hasAttribute("disablehistory");
> > Ah, now this was the ugliest one, but not forgetting...
> I'm not sure I see your point. Are you saying that we shouldn't use that
> language feature at all?
Oh, I can see its usefulness in 1-line anonymous callbacks, because the idea there is that removing the braces improves readability.

Comment 8

[[e-mail]]

Comment on attachment 404199 [details] [diff] [review]
patch v3

>@@ -289,34 +245,37 @@ function onLoadContent()
>+  if (isHistoryEnabled())
>+    UpdateBackForwardCommands();
Does that actually work with bfcache, or does bfcache not apply here?

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #8)
> (From update of attachment 404199 [details] [diff] [review])
> >@@ -289,34 +245,37 @@ function onLoadContent()
> >+  if (isHistoryEnabled())
> >+    UpdateBackForwardCommands();
> Does that actually work with bfcache, or does bfcache not apply here?

It does apply here, as far as I can tell, and onLoadContent gets called correctly.

Comment 10

[[e-mail]]

(In reply to comment #9)
>(In reply to comment #8)
>>(From update of attachment 404199 [details] [diff] [review] [details])
>>>@@ -289,34 +245,37 @@ function onLoadContent()
>>>+  if (isHistoryEnabled())
>>>+    UpdateBackForwardCommands();
>>Does that actually work with bfcache, or does bfcache not apply here?
>It does apply here, as far as I can tell, and onLoadContent gets called
>correctly.
I'm not seeing bfcache get invoked, which is why onLoadContent gets called.

Comment 11

[[e-mail]]

To be specific, the first call to CanSavePresentation (from InternalLoad) returns true, but the second call (from CreateContentViewer) returns false, ostensibly because of a pending request for console.properties!

Comment 12

[[e-mail]]

(In reply to comment #11)
> To be specific, the first call to CanSavePresentation (from InternalLoad)
> returns true, but the second call (from CreateContentViewer) returns false,
> ostensibly because of a pending request for console.properties!
Well, that request is expected, because that was the link I clicked on, duh.
But I don't see why the old document is seeing it in its load group.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> ostensibly because of a pending request for console.properties!

This is view-source, so the loadgroup will have at least two requests in it: the view-source channel and the underlying (http, file, whatever) channel.  So as things stand, this will never get bfcached.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Oh, and the loadgroup is per-docshell, not per-document, which is why we need the whole "check whether the loadgroup happens to only have the one request for the new document we're loading" hack.

Comment 15

[[e-mail]]

Comment on attachment 404199 [details] [diff] [review]
patch v3

OK, so I think we need a big comment in case someone makes view source documents cacheable.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Is there a reason to not use pageshow/pagehide instead?

Comment 17

[Dão Gottwald [:dao]]

landed with pageshow/pagehide:

http://hg.mozilla.org/mozilla-central/rev/0a7dd88dbe67

Comment 18

[[e-mail]]

Might have been worth renaming the methods too, but nm.