Bug 38635 – Web Inspector: Do not show content tab for resources not supporting content preview.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Pavel Feldman]

Patch to follow.

[Pavel Feldman]

Created attachment 55220 [details]
[PATCH] Proposed change.

[Pavel Feldman]

Created attachment 55221 [details]
[PATCH] Same with binary diff for removed localized string.

[Timothy Hatcher]

Comment on attachment 55221 [details]
[PATCH] Same with binary diff for removed localized string.

So what views/resource types will no have a content tab now?

[Pavel Feldman]

(In reply to comment #3)
> (From update of attachment 55221 [details])
> So what views/resource types will no have a content tab now?

Resources that had ResourceView as a renderer (and resulted in "No content available" at all times) will have no content tab. No other changes here.

[WebKit Commit Bot]

Comment on attachment 55221 [details]
[PATCH] Same with binary diff for removed localized string.

Rejecting patch 55221 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Timothy Hatcher', u'--force']" exit_code: 255
Parsed 6 diffs from patch file(s).
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
only literal type is supported now at /Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply line 254.

Full output: http://webkit-commit-queue.appspot.com/results/2161024

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/English.lproj/localizedStrings.js
	M	WebCore/inspector/front-end/FontView.js
	M	WebCore/inspector/front-end/ImageView.js
	M	WebCore/inspector/front-end/ResourceView.js
	M	WebCore/inspector/front-end/SourceView.js
Committed r58884

[Joseph Pecoraro]

>  WebInspector.FontView.prototype = {
> +    hasContentTab: function()
> +    {
> +        return true;
> +    },
> +

Why a function and not just a boolean?

  hasContentTab: true,

Then those that don't include the boolean would still work (undefined ~~ false).


> @@ -118,6 +119,8 @@ WebInspector.ResourceView = function(resource)
>      this._refreshRequestHeaders();
>      this._refreshResponseHeaders();
>      this._refreshHTTPInformation();
> +    if (!this.hasContentTab())
> +        this.contentTabElement.addStyleClass("hidden");
>      this._selectTab();

How does this affect tab preferences? If you want to see the content, and you click (1) resource with preview, (2) resource without preview, (3) resource with preview?


> +    hasContentTab: function()
> +    {
> +        // Ancestors should override this method and define this.contentTabSelected for content rendering.
> +        return false;
> +    },

I think "Ancestors" is the wrong direction. "Children" or "Sub-something" might be better.

[Timothy Hatcher]

Subclasses.

[Pavel Feldman]

(In reply to comment #7)
> >  WebInspector.FontView.prototype = {
> > +    hasContentTab: function()
> > +    {
> > +        return true;
> > +    },
> > +
> 
> Why a function and not just a boolean?

Well, I want it to be immutable.

> How does this affect tab preferences? If you want to see the content, and you
> click (1) resource with preview, (2) resource without preview, (3) resource
> with preview?
> 

I think it'll reset preferences to Headers in that case. Now sure which way is more predictable though.

> 
> I think "Ancestors" is the wrong direction. "Children" or "Sub-something" might
> be better.

I surely meant derived classes.

[Joseph Pecoraro]

> > Why a function and not just a boolean?
> 
> Well, I want it to be immutable.

Bad argument. Making this a function doesn't protect against this. Both are easily mutable.

>  WebInspector.FontView.prototype.hasContentTab = function() { return false; }


> I think it'll reset preferences to Headers in that case. Now sure which way is
> more predictable though.

I certainly wouldn't like that! I think I'd prefer seeing "No Content" instead.

[Pavel Feldman]

> 
> > I think it'll reset preferences to Headers in that case. Now sure which way is
> > more predictable though.
> 
> I certainly wouldn't like that! I think I'd prefer seeing "No Content" instead.

"No Content" is bad because "Content" tab is being default even for entries with no content. I was demoing inspector's redirects functionality and it was very silly of resources tab to default to "Content" for "Moved permanently" resources having no content. At the same time, preserving "Content" as last selected while traversing through content-less entries should be feasible.