Tim Taubert [:ttaubert] (inactive) Reporter
	Description • 11 years ago

Needed as a prerequisite for bug 583890.

	Adam
	Comment 1 • 11 years ago

This patch looks fine to me. Wish the other one were this simple.

	Matthew N. [:MattN]
	Comment 2 • 11 years ago

Comment on attachment 8339633 [details] [diff] [review]
Moved test to its new bug number

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

::: browser/base/content/tabbrowser.xml
@@ +1553,5 @@
>              // initialized by this point.
>              this.mPanelContainer.appendChild(notificationbox);
>  
> +            // Happens after the browser is in the DOM: the TabTitleAbridger
> +            // classifies tabs by domains, requiring access to the browser.

It would be nice to have a test that ensures that the browser is accessible when the event is fired.

@@ +1554,5 @@
>              this.mPanelContainer.appendChild(notificationbox);
>  
> +            // Happens after the browser is in the DOM: the TabTitleAbridger
> +            // classifies tabs by domains, requiring access to the browser.
> +            if (!aURI || isBlankPageURL(aURI)) {

Drive-by feedback: I think the comment should be a bit more clear by indicating that consumers of TabLabelModified (such as TabTitleAbridger) may need access to the browser because as-is the comment didn't make sense to me since I didn't see the TabTitleAbridger in the patch and wondered what was calling it. A proper sentence would also be preferred.

	Adam
	Comment 3 • 11 years ago

Thanks Matt!

The first few tests already fail if a new tab's label is set before the tab element is fully initialized, but the new test should defend the need for the linkedBrowser explicitly.

	Tim Taubert [:ttaubert] (inactive) Reporter
	Comment 4 • 11 years ago

Comment on attachment 8339678 [details] [diff] [review]
Improve comment, add linkedBrowser accessibility test

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

::: browser/base/content/tabbrowser.xml
@@ +1555,5 @@
>  
> +            // The label needs to be set after the browser is in the DOM.
> +            // This is because consumers of the TabLabelModified event
> +            // (such as TabTitleAbridger) need information that is with the
> +            // browser (such as the domain/URI of the tab).

Nit: don't mention the TTA, we're creating a generic API :)

And actually, we're not setting the label after the tab has been appended so the event handler can access the tab's browser but rather so that event can be dispatched at all and will bubble up to any event handler registered on the tabContainer.

@@ +4692,5 @@
> +          });
> +          this.dispatchEvent(event);
> +
> +          if (!event.defaultPrevented)
> +            this.visibleLabel = val;

Please add a comment to say that we're allowing listeners to cancel overriding a custom tab label.

::: browser/base/content/test/general/browser.ini
@@ +230,5 @@
>  [browser_bug887515.js]
>  [browser_bug896291_closeMaxSessionStoreTabs.js]
>  [browser_bug902156.js]
>  [browser_bug906190.js]
> +[browser_bug943820.js]

The test should have a better name, browser_visibleLabel.js maybe?

::: browser/base/content/test/general/browser_bug943820.js
@@ +12,5 @@
> +  waitForExplicitFinish();
> +  registerCleanupFunction(function() {
> +    gBrowser.removeCurrentTab({animate: false});
> +  });
> +  let tab = gBrowser.addTab("about:newtab",

I'd rather use about:blank here, about:newtab may not fire a load event at all when it was preloaded.

@@ +16,5 @@
> +  let tab = gBrowser.addTab("about:newtab",
> +                            {skipAnimation: true});
> +  tab.linkedBrowser.addEventListener("load", function onLoad(event) {
> +    event.currentTarget.removeEventListener("load", onLoad, true);
> +    gBrowser.selectedTab = tab;

You can do |let tab = gBrowser.selectedTab = gBrowser.addTab(...)|.

@@ +23,5 @@
> +  tab.addEventListener("TabLabelModified", handleInTest, true);
> +}
> +
> +// Prevent interference
> +function handleInTest(aEvent) {

When landed without the TabTitleAbridger this doesn't really make sense. There is nothing that interferes here (yet) and we always set visibleLabel=label anyway. Once bug 583890 lands it should probably fix this test by disabling itself for the runtime of the test.

@@ +66,5 @@
> +  tab.addEventListener("TabLabelModified", handleTabLabel, true);
> +  tab.label = "This won't be the visibleLabel";
> +}
> +
> +function handleTabLabel(aEvent) {

Would "overrideTabLabel" be a better name?

@@ +93,5 @@
> +
> +function checkBrowserAccessibilityOnNewTab() {
> +  gBrowser.tabContainer.addEventListener("TabLabelModified",
> +    handleBrowserAccessibilityTest, true);
> +  let tab = gBrowser.addTab("about:newtab", {skipAnimation: true});

about:blank

@@ +100,5 @@
> +function handleBrowserAccessibilityTest(aEvent) {
> +  gBrowser.tabContainer.removeEventListener("TabLabelModified",
> +    handleBrowserAccessibilityTest, true);
> +  ok(aEvent.target.linkedBrowser,
> +    "Linked browser is accessible by the event handler");

That check seems superfluous. The event was dispatched and handled so we know the tab was in the tree before it fired the event.

	Adam
	Comment 5 • 11 years ago

Thanks, Tim!

> @@ +100,5 @@
> > +function handleBrowserAccessibilityTest(aEvent) {
> > +  gBrowser.tabContainer.removeEventListener("TabLabelModified",
> > +    handleBrowserAccessibilityTest, true);
> > +  ok(aEvent.target.linkedBrowser,
> > +    "Linked browser is accessible by the event handler");
> 
> That check seems superfluous. The event was dispatched and handled so we know
> the tab was in the tree before it fired the event.

I've replaced it with an |ok(true, ...);| to note success there.

	Tim Taubert [:ttaubert] (inactive) Reporter
	Comment 6 • 11 years ago

Comment on attachment 8340147 [details] [diff] [review]
Update comments, use about:blank in tests, address other feedback

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

Looks good to me, thanks! Did you push it to try yet?

	Adam
	Comment 7 • 11 years ago

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=d76973f66170

	Tim Taubert [:ttaubert] (inactive) Reporter
	Comment 8 • 11 years ago

https://hg.mozilla.org/integration/fx-team/rev/089349127563

	Carsten Book [:Tomcat]
	Comment 9 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/089349127563

	Ioana (away)
	Comment 10 • 11 years ago

browser/base/content/test/general/browser_visibleLabel.js

	Dão Gottwald [:dao]
	Comment 11 • 9 years ago

I backed this out in bug 1247920.