Closed Bug 1418403 Opened 7 years ago Closed 6 years ago

Remove view_source.tab pref

Categories

(Toolkit :: View Source, enhancement, P3)

Product:
Toolkit
Component:
View Source
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
relnote-firefox --- -
firefox60 --- fixed

People

(Reporter: bgrins, Assigned: bdahl)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

Bug 1418403 - Remove old view source window title test.
59 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1418403 - Remove viewing source in a standalone window.
59 bytes, text/x-review-board-request
jryans
: review+
Details 
This was mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1416372#c3, so spinning it off into a new bug.

From https://bugzilla.mozilla.org/show_bug.cgi?id=1416372#c2:

When we collected data on the different modes 2 years ago (bug 1203624 comment 11), we saw:

View Source in Browser:         56,700 uses (99.30%)
View Source in Window:             357 uses ( 0.62%)
View Source External (Success):     44 uses ( 0.08%)
View Source External (Failed):      48 uses (re-counted as browser / window above)

Maybe these numbers are low enough that we should remove the pref and the additional code to support both modes.
Severity: normal → enhancement
Priority: -- → P3
Assignee: nobody → bdahl
Comment on attachment 8948810 [details]
Bug 1418403 - Remove viewing source in a standalone window.

https://reviewboard.mozilla.org/r/218176/#review224002


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/viewsource/test/browser/browser_gotoline.js:22
(Diff revision 1)
> -  await BrowserTestUtils.closeWindow(win);
> +  gBrowser.removeTab(tab);
>  });
>  
> -var checkViewSource = async function(aWindow) {
> -  is(aWindow.gBrowser.contentDocument.body.textContent, content, "Correct content loaded");
> +var checkViewSource = async function(aTab) {
> +  let browser = aTab.linkedBrowser;
> +  is(browser.contentDocument.body.textContent, content, "Correct content loaded");

Error: Browser.contentdocument.body.textcontent is a possible cross process object wrapper (cpow). [eslint: mozilla/no-cpows-in-tests]
Comment on attachment 8948809 [details]
Bug 1418403 - Remove old view source window title test.

https://reviewboard.mozilla.org/r/218174/#review224250

Thanks, removing this test seems fine.
Attachment #8948809 - Flags: review?(jryans) → review+
Comment on attachment 8948810 [details]
Bug 1418403 - Remove viewing source in a standalone window.

https://reviewboard.mozilla.org/r/218176/#review224252

First off, thanks for working on this!

Now that I've seen the patch, I am realizing there's a concern here that hasn't been discussed yet.  The current view source code lives in /toolkit and doesn't really assume a specific application.  For Firefox, the default path is to show source in a tab, and we want to remove the old path of using a XUL window.  For other XUL apps like Thunderbird[1], they currently call into at least `viewSource` and `viewPartialSourceInBrowser` in their own code.  For example, when I try Thunderbird locally, you can go to Tools -> Web Developer -> Error Console and click on the linked source file for an error, which opens the view source window we are removing here.

With your changes as-is, we start embedding the assumption that the application is Firefox into `viewSource`.  Now, I don't think we want to delay XUL / XBL cleanup for other apps, so let's think about the right approach here.  Since we want to remove the view source window, I am not sure there's a clear path for what we would actually do in non-Firefox anymore.  One option would be to stop attempting maintain this as a /toolkit module for any app and instead move the code to Firefox, where we are free to make application assumptions.  Other apps could copy the /toolkit as it is for their use moving forward.  

However, that's just one option, so there might be a better way.  :bdahl, what are your thoughts about this?  Also, :mconley may have an opinion, so I'll flag him as well.

(I am sorry that this became quite a bit more complex that I imagined at first!)

[1]: https://searchfox.org/comm-central/search?q=gViewSourceUtils&case=false&regexp=false&path=
Attachment #8948810 - Flags: review?(jryans) → review-
:mconley, you may have an opinion on comment 6 above.
Flags: needinfo?(mconley)
(Spoken as somebody who has read comment 6 and only quickly glanced at the patch)

Yeah, I think this is going to break things in Thunderbird and SeaMonkey. I totally understand that we want to stop supporting View Source in a Window, but I think that means we can just make changes in how Firefox uses these toolkit APIs. If those toolkit APIs are problematic, we should probably fork them into the browser/ folder and start using our own, and leave the toolkit ones alone. The only reason I can think of for getting rid of the toolkit code is:

1) The code is an undue maintainance burden on us that we need to eliminate. In that case, we should talk to the TB / SM communities about moving the code into comm-central
2) The code is being replaced with something better that the other XUL apps can use

TB and SM's priorities and needs cannot supercede Firefox's when it comes to Gecko and Toolkit - however, if we can easily avoid breaking them without incurring too much in the way of penalties on our side, that might be the best route.

So if we're dead-set on removing this from mozilla-central, I believe we should open communications with TB / SM to move the code into comm-central. If we're not dead-set on removing the code from mozilla-central, I believe we should just have browser/ stop using it and leave it in toolkit/ for now. That's my 2c.
Flags: needinfo?(mconley)
(In reply to Mike Conley (:mconley) (:⚙️) from comment #8)
> (Spoken as somebody who has read comment 6 and only quickly glanced at the
> patch)
> 
> Yeah, I think this is going to break things in Thunderbird and SeaMonkey. I
> totally understand that we want to stop supporting View Source in a Window,
> but I think that means we can just make changes in how Firefox uses these
> toolkit APIs. If those toolkit APIs are problematic, we should probably fork
> them into the browser/ folder and start using our own, and leave the toolkit
> ones alone. The only reason I can think of for getting rid of the toolkit
> code is:
> 
> 1) The code is an undue maintainance burden on us that we need to eliminate.
> In that case, we should talk to the TB / SM communities about moving the
> code into comm-central
> 2) The code is being replaced with something better that the other XUL apps
> can use
> 
> TB and SM's priorities and needs cannot supercede Firefox's when it comes to
> Gecko and Toolkit - however, if we can easily avoid breaking them without
> incurring too much in the way of penalties on our side, that might be the
> best route.
> 
> So if we're dead-set on removing this from mozilla-central, I believe we
> should open communications with TB / SM to move the code into comm-central.
> If we're not dead-set on removing the code from mozilla-central, I believe
> we should just have browser/ stop using it and leave it in toolkit/ for now.
> That's my 2c.

Since we are actively working to remove all overlays in Firefox I don't think leaving viewSourceOverlay as-is in toolkit would be a viable solution.

It does look like comm-central keeps its own copies of viewSourceOverlay.xul (https://searchfox.org/comm-central/search?q=viewsourceoverlay.xul&path=), so if they restored viewSource.xul and the code in viewSourceUtils.js that points to it, then it should continue working. This seems pretty similar to what we've been doing with a bunch of XBL bindings that get removed in m-c but are still used in c-c (i.e. xpfe autocomplete in Bug 1418512).

As for if we should move the rest of viewsource from toolkit/ into browser/, I'm not sure. The line between them can be pretty blurry and as you point out there are a lot of Firefox-specific assumptions in the code - maybe Mossop has an opinion.
Flags: needinfo?(dtownsend)
We can't let Thunderbird/Seamonkey hold us back here. If the code in question isn't relevant for Firefox/Fennec then it doesn't belong in m-c and we should feel free to remove it. If c-c want it then they can fork it.
Flags: needinfo?(dtownsend)
Thanks all for the input!  Sounds like we are okay with embedding Firefox assumptions in this /toolkit code, then.  I'll put :bdahl patch back in my queue.
Like Brian said, the goal here is to remove a XUL overlay. This could be done in other ways, but I still think removing this code is the best approach: 1) it removes the overlay 2) it removes a feature that is barely used 3) adds more testing (previously, half the tests were running for window mode only) 4) reduces complexity.

As for not creating a dependency on browser in toolkit. I originally started on a branch where I removed gViewSourceUtils.viewSource entirely, but I brought it back to support some of the devtools uses of it for the browser toolbox. It seems that's the only piece that relies on browser specific code, so I could just move that over to browser, but leave the rest of gViewSourceUtils. In the long run it'd be nice to have a single API for view source though.

Also, we could benefit from a second round of cleanup, but I don't want this to get bigger than it already is.
Comment on attachment 8948810 [details]
Bug 1418403 - Remove viewing source in a standalone window.

https://reviewboard.mozilla.org/r/218176/#review224398

Thanks for working on this! :) Since we've agreed we're okay with Firefox specifics in /toolkit, this looks good to me overall.

I agree there could be a second pass to fold what's left here more directly into /browser, but that can be left to a follow up.

::: toolkit/components/viewsource/content/viewSourceUtils.js:67
(Diff revision 2)
> +          topic !== "domwindowopened") {
> +        return;
> -    }
> +      }
> +      Services.ww.unregisterNotification(onOpen);
> +      let win = subj.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> +                     .getInterface(Components.interfaces.nsIDOMWindow);

Nit: align with dot above

::: toolkit/components/viewsource/content/viewSourceUtils.js:265
(Diff revision 2)
>        this.handleCallBack(aCallBack, false, data);
>      }
>    },
>  
>    // Default callback - opens the internal viewer if the external editor failed
>    internalViewerFallback(result, data) {

This should also be removed since `_openInInternalViewer` is gone now.

::: toolkit/components/viewsource/test/browser/head.js:14
(Diff revision 2)
>   * Wait for view source tab or window after calling given function to open it.
>   *
>   * @param open - a function to open view source.
>   * @returns the new tab or window which shows the source.
>   */
>  async function waitForViewSourceTabOrWindow(open) {

Please rename this to drop the `OrWindow` suffix.  (Function docs and local variable names here need a bit of clean up as well.)

::: toolkit/components/viewsource/test/browser/head.js:51
(Diff revision 2)
> + * Opens a view source tab. (View Source)
> + * within the currently selected browser in gBrowser.
> + *
> + * @returns the new tab / window which shows the source.
> + */
> +async function openViewFullSource() {

What does `Full` mean here?  Is there a more meaningful name we can use instead?
Attachment #8948810 - Flags: review?(jryans) → review+
Comment on attachment 8948810 [details]
Bug 1418403 - Remove viewing source in a standalone window.

https://reviewboard.mozilla.org/r/218176/#review224398

> What does `Full` mean here?  Is there a more meaningful name we can use instead?

Full means just the regular view source of a whole page (opposed to openViewPartialSource function in this file). I mainly chose that name since openViewSource() was already used within the file. To make it more clear, I've renamed to openViewFullSource() to openViewSource() and openViewSource() to openViewSourceForBrowser().
Comment on attachment 8948810 [details]
Bug 1418403 - Remove viewing source in a standalone window.

https://reviewboard.mozilla.org/r/218176/#review225422


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/viewsource/test/browser/head.js:177
(Diff revision 3)
> +  let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, aURI);
> +  registerCleanupFunction(function() {
> +    gBrowser.removeTab(tab);
> +  });
> +
> +  return openViewSource();

Error: 'openviewsource' is not defined. [eslint: no-undef]
Comment on attachment 8948810 [details]
Bug 1418403 - Remove viewing source in a standalone window.

https://reviewboard.mozilla.org/r/218176/#review225858


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/viewsource/content/viewSourceUtils.js:187
(Diff revision 5)
> -        data.doc = {
> +      data.doc = {
> -          characterSet: browser.characterSet,
> +        characterSet: browser.characterSet,
> -          contentType: browser.documentContentType,
> +        contentType: browser.documentContentType,
> -          title: browser.contentTitle,
> +        title: browser.contentTitle,
> -        };
> +      };
> -        data.isPrivate = PrivateBrowsingUtils.isBrowserPrivate(browser);
> +      data.isPrivate = PrivateBrowsingUtils.isBrowserPrivate(browser);

Error: 'privatebrowsingutils' is not defined. [eslint: no-undef]
Blocks: 1392352
Comment on attachment 8948810 [details]
Bug 1418403 - Remove viewing source in a standalone window.

https://reviewboard.mozilla.org/r/218176/#review226658

::: toolkit/components/viewsource/test/browser/browser_contextmenu.js:37
(Diff revision 6)
> -  await new Promise(resolve => {
> -    closeViewSourceWindow(newWindow, resolve);
> -  });
>  });
>  
>  async function onViewSourceWindowOpen(aWindow, aIsTab) {

You should be able to remove the `aIsTab` arg and associated code / callers

::: toolkit/components/viewsource/test/browser/browser_contextmenu.js:55
(Diff revision 6)
>    expectedData.push(["a[href]", true, false, "http://example.com/"]);
>    expectedData.push(["a[href^=mailto]", false, true, "abc@def.ghi"]);
>    expectedData.push(["span", false, false, null]);
>  }
>  
>  async function checkMenuItems(contextMenu, isTab, selector, copyLinkExpected, copyEmailExpected, expectedClipboardContent) {

Ditto
Pushed by bdahl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a7f80d59de44
Remove old view source window title test. r=jryans
https://hg.mozilla.org/integration/autoland/rev/fcfdf000a8f3
Remove viewing source in a standalone window. r=jryans
https://hg.mozilla.org/mozilla-central/rev/a7f80d59de44
https://hg.mozilla.org/mozilla-central/rev/fcfdf000a8f3
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Why wasn't this at least communicated?  I just got updated to a nightly with this change, and it completely breaks all sorts of workflow for me.  For starters, I want different sizes for my view-source windows and my web-browsing windows, because the use cases are totally different...

But more importantly, at the time we added the in-tab mode there were explicit guarantees given that we would not remove the old mode without clear communication and feature parity.  And we're not at feature parity.  Most simply, there "go to line" keyboard shortcut went away and got replaced by a hard-to-reach context menu item!
To be clear, I am very sympathetic with the goal of removing codepaths.  If we had a keyboard shortcut for "go to line" and if I could have the source open in a separate window by default so I can by default see both the page and the source, even if that separate window is just a normal browser window, that would help a lot from my point of view...  It still wouldn't deal with the "want different sizes" issue, but I can survive.
OK.  There is apparently a keyboard shortcut, but it's broken with certain accessibility settings: bug 1171316.  So not usable for me. :(

I filed bug 1444133 on the other part of comment 27.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #26)
> Why wasn't this at least communicated?  I just got updated to a nightly with
> this change, and it completely breaks all sorts of workflow for me.  For
> starters, I want different sizes for my view-source windows and my
> web-browsing windows, because the use cases are totally different...
> 
> But more importantly, at the time we added the in-tab mode there were
> explicit guarantees given that we would not remove the old mode without
> clear communication and feature parity.  And we're not at feature parity. 
> Most simply, there "go to line" keyboard shortcut went away and got replaced
> by a hard-to-reach context menu item!

I apologize about the communication mismatch here...  If I was one the making such promises back then, they escaped my memory.  I am aware of the bugs filed about missing features for the in-tab version.  At the current moment in time, they did not seem critical enough to block XBL removal before being resolved.

(I do recall a discussion about ensuring View Source is not replaced by some UI that can't handle giant source files, and AFAIK the in-tab version performs similarly to the window version in this regard.)

Let's continue discussing on IRC and in this bug.  I'll mark this bug as a relnote and dev-doc-needed.  Let me know if additional messaging seems needed.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #27)
> To be clear, I am very sympathetic with the goal of removing codepaths.  If
> we had a keyboard shortcut for "go to line" and if I could have the source
> open in a separate window by default so I can by default see both the page
> and the source, even if that separate window is just a normal browser
> window, that would help a lot from my point of view...  It still wouldn't
> deal with the "want different sizes" issue, but I can survive.

For "Go to Line", we do have a shortcut in tab (Ctrl-Option-L / Alt-Shift-L), but you had previously encountered it being blocked by some accessibility settings (bug 1171316).  Perhaps we can use that bug to work out a path forward on that front.

For opening in a separate window, let's discuss it.  Ah, looks like you have just filed bug 1444133.  Thanks!
> Let me know if additional messaging seems needed.

I think a post to .platform is probably a good idea.
Release Note Request (optional, but appreciated)
[Why is this notable]: A feature was removed, some developers may be confused by its disappearance.
[Affects Firefox for Android]: No
[Suggested wording]: View Source now always opens in a browser tab.  The traditional View Source window has been removed.
[Links (documentation, blog post, etc)]: None so far.
relnote-firefox: --- → ?
Keywords: dev-doc-needed
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #30)
> > Let me know if additional messaging seems needed.
> 
> I think a post to .platform is probably a good idea.

Okay, message posted: https://groups.google.com/d/topic/mozilla.dev.platform/_qX9P9tBz2I/discussion
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #13)
> I agree there could be a second pass to fold what's left here more directly
> into /browser, but that can be left to a follow up.

I'm a bit late to the party, but would like to note that as of recently this is used on Android as well, so /browser wouldn't be the right choice.
I've documented this, by adding notes about it to the main View Source page and the Fx 60 rel notes:

https://developer.mozilla.org/en-US/docs/Tools/View_source
https://developer.mozilla.org/en-US/Firefox/Releases/60#Developer_tools

Let me know if that looks OK. Thanks!
Flags: needinfo?(bdahl)
Keywords: dev-doc-needed → dev-doc-complete
Looks good, thanks!
Flags: needinfo?(bdahl)
This seems too obscure for the main user-facing release notes.
relnote-firefox: ? → -
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: