Closed Bug 1416372 Opened 4 years ago Closed 4 years ago

See if we can remove the <statusbar> and associated bindings from view source (which appears to be the only consumer of the associated XBL / accessibility code)

Categories

(Toolkit :: View Source, task, P3)

Product:
Toolkit
Component:
View Source
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(2 files, 1 obsolete file)

xbl-statusbar.png
191.78 KB, image/png
Details
Bug 1416372 - Remove remaining references to the view source statusbar;
59 bytes, text/x-review-board-request
jryans
: review+
Details
Attached image xbl-statusbar.png — 
There are a set of bindings that AFAICT are only ever used in a view source window when it's not loaded in a tab.

STR:

./mach run --setpref view_source.tab=false (or open a firefox profile and flip view_source.tab)
Open mozilla.org
Right Click -> View Source

It's the space that shows the selected line/col in the window (see screenshot). I don't see any other consumers of this binding: https://dxr.mozilla.org/mozilla-central/search?q=%3Cstatusbar.

There's accessibility code in addition to the XBL: https://dxr.mozilla.org/mozilla-central/search?q=path%3Aaccessible&redirect=false. And maybe widget code - not sure if this is related: https://dxr.mozilla.org/mozilla-central/search?q=path%3Awidget%2F+statusbar&redirect=true.
I guess the main question here is if we are OK with removing that piece of UI from the view source window.

AFAICT the window is only ever shown for people who have changed the pref value to a non-default value, but maybe I am missing another entry point. Ryan, are you aware of another way that you'd end up seeing this statusbar: https://searchfox.org/mozilla-central/source/toolkit/components/viewsource/content/viewSource.xul#232 ?
Flags: needinfo?(jryans)
(In reply to Brian Grinstead [:bgrins] from comment #1)
> I guess the main question here is if we are OK with removing that piece of
> UI from the view source window.
> 
> AFAICT the window is only ever shown for people who have changed the pref
> value to a non-default value, but maybe I am missing another entry point.
> Ryan, are you aware of another way that you'd end up seeing this statusbar:
> https://searchfox.org/mozilla-central/source/toolkit/components/viewsource/
> content/viewSource.xul#232 ?

Correct, the non-default view source in window mode is the only way you would see that.

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)

I guess Harald is the closest we have to a PM for View Source (since it's a tool for developers, even though it's not really thought of as owned by the DevTools team).

Harald, any opinions on whether this proposed removal seems safe?

On my end, I would say it seems safe to proceed with removal.  The alternative would be to reimplement it somehow I guess, but that feels like time better spent elsewhere, given the usage data here.
Flags: needinfo?(jryans) → needinfo?(hkirschner)
Severity: normal → enhancement
Priority: -- → P3
Given the default this seems safe. Also the numbers 2 years ago had such low usage, it is probably negligible today. I assume the pref will be removed at the same time?
Flags: needinfo?(hkirschner)
(In reply to :Harald Kirschner :digitarald from comment #3)
> Given the default this seems safe. Also the numbers 2 years ago had such low
> usage, it is probably negligible today. I assume the pref will be removed at
> the same time?

I think it makes sense to do in separate bugs.  I'll file one to just remove the pref in case there more discussion surfaces about that.
See Also: → 1418403
Ryan, would you prefer we remove the <statusbar><statusbarpanel /></statusbar> in viewSource.xul or convert the markup into something like <box><label /></box>?
Flags: needinfo?(jryans)
(In reply to Brian Grinstead [:bgrins] from comment #5)
> Ryan, would you prefer we remove the <statusbar><statusbarpanel
> /></statusbar> in viewSource.xul or convert the markup into something like
> <box><label /></box>?

Pushed up a patch with the former but I can change it into the latter if you prefer
(In reply to Brian Grinstead [:bgrins] from comment #5)
> Ryan, would you prefer we remove the <statusbar><statusbarpanel
> /></statusbar> in viewSource.xul or convert the markup into something like
> <box><label /></box>?

I think it's reasonable to proceed with removal for this disabled feature.  If we happen to learn through this change that many people rely on this statusbar, then I think that would be evidence that it should be added to the enabled version in tab (instead of recreated in the disabled window one).
Flags: needinfo?(jryans)
Comment on attachment 8930165 [details]
Bug 1416372 - Remove line line:col display in separate view source windows;

https://reviewboard.mozilla.org/r/201318/#review206430
Attachment #8930165 - Flags: review?(jryans) → review+
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05fc5e9fd820
Remove line line:col display in separate view source windows;r=jryans
FYI, there’s still a reference to the statusbar in https://hg.mozilla.org/integration/autoland/file/05fc5e9fd820/toolkit/components/viewsource/content/viewSource.js#l608 at lines 608-619, it might be a good idea to remove it there as well.
Flags: needinfo?(bgrinstead)
(In reply to ExE Boss from comment #12)
> FYI, there’s still a reference to the statusbar in
> https://hg.mozilla.org/integration/autoland/file/05fc5e9fd820/toolkit/
> components/viewsource/content/viewSource.js#l608 at lines 608-619, it might
> be a good idea to remove it there as well.

Thanks for noticing. Going to leave-open so I can get that cleaned up.
Flags: needinfo?(bgrinstead)
Keywords: leave-open
There was more code than I thought there would be, mostly related to selection tracking in the content script. Pushed up a patch that removes it all and have a try push going to confirm this didn't break anything else: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48c5380babb11e65858e5a2c4294b0bc487d2193
Blocks: 1419170
Comment on attachment 8930245 [details]
Bug 1416372 - Remove remaining references to the view source statusbar;

https://reviewboard.mozilla.org/r/201374/#review206500

Thanks, sorry for missing that earlier!  Hadn't had enough coffee yet or something I guess... :)

::: toolkit/components/viewsource/content/viewSource.js:612
(Diff revision 1)
>     *
>     * @param lineNumber
>     *        The line number that we successfully got to.
>     */
>    onGoToLineSuccess(lineNumber) {
>      ViewSourceBrowser.prototype.onGoToLineSuccess.call(this, lineNumber);

This should also be safe to remove now.  It's effectively a `super` call to its parent class, and the previous patch removed the extra code here that made this version different.
Attachment #8930245 - Flags: review?(jryans) → review+
Attachment #8930165 - Attachment is obsolete: true
Keywords: leave-open
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ab0a609b4db5
Remove remaining references to the view source statusbar;r=jryans
https://hg.mozilla.org/mozilla-central/rev/ab0a609b4db5
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.