Bug 33186 – Track state for whether a ResourceRequest is fetched via SPDY

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mike Belshe]

Track whether a ResourceRequest is fetched via the SPDY protocol.  Initially this will be chromium specific.
SPDY is part of an experiment to see if we can't build a faster web protocol:  http://dev.chromium.org/spdy.

[Mike Belshe]

Created attachment 45913 [details]
patch

[WebKit Review Bot]

Attachment 45913 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/WebURLResponse.cpp:258:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebKit/chromium/src/WebURLResponse.cpp:262:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2

[Mike Belshe]

Created attachment 45921 [details]
patch fixing style nits

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45921 [details] without any errors.

[Eric Seidel (no email)]

I don't really understand the "isSPDY" naming scheme here.  Then again, I don't really know much about SPDY.  Does the protocol name change for SPDY?  like "spdy://"?  I'm just wondering if this is the right way to track this, and if "isSPDY" is the right name to do so.

[Mike Belshe]

Eric - the protocol scheme doesn't change, so there are no visible effects to see whether or not SPDY was used.  With this, I plan to enable some JS so that the page author can determine whether SPDY was used to deliver the page or not.

[David Levin]

Two comments:
1. It seems redundant that there is #if PLATFORM(CHROMIUM) inside of the chromium only file WebCore/platform/network/chromium/ResourceResponse.h

2. Note that this flag will not be copied for workers because that is done in WebCore/platform/network/ResourceResponseBase.h  I don't know if that is important to this change (or will be in the future). If it is, then I can tell you how to fix this.

[Mike Belshe]

Created attachment 45977 [details]
Remove ifdef CHROMIUM flag as per David's suggestion

[Mike Belshe]

Thanks David - I removed the ifdef.  This isn't needed in the Workers - I am only going to expose this for the main frame URLs anyway.  If I need changes in the future, we can add that.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45977 [details] without any errors.

[Darin Fisher (:fishd, Google)]

Comment on attachment 45977 [details]
Remove ifdef CHROMIUM flag as per David's suggestion

> Index: WebCore/platform/network/chromium/ResourceResponse.h
...
> +        // If this request was fetched via SPDY.
> +        bool m_isSpdy;

How about naming this m_fetchedViaSPDY?  Then, the comment can be
deleted.  You should similarly rename the getter and setter.

Same goes for the corresponding WebURLResponse methods.

With those changes, LGTM.

[Darin Adler]

Comment on attachment 45977 [details]
Remove ifdef CHROMIUM flag as per David's suggestion

For boolean data and function members we try to make them work as if they were in this sentence: "response <xxx>". So something like wasFetchedViaSPDY would be an example of a good name.

[Mike Belshe]

Created attachment 46001 [details]
Update with Darin's renaming suggestions.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46001 [details] without any errors.

[Darin Adler]

(In reply to comment #13)
> Update with Darin's renaming suggestions.

You mean Darins’ renaming suggestions ;-)

[Mike Belshe]

yes! :-)

[Darin Adler]

Comment on attachment 46001 [details]
Update with Darin's renaming suggestions.

If the name is SPDY, the we would prefer to have that in the identifiers, not Spdy.

> +        Enabled only in Chromium.

No longer true.

> +        * platform/network/chromium/ResourceResponse.h:
> +        (WebCore::ResourceResponse::isSpdy):
> +        (WebCore::ResourceResponse::setIsSpdy):

Names here out of date.

Where is m_fetchedViaSpdy initialized to false? Normally constructors should initialize all members.

> +        void setWasFetchedViaSpdy(const bool value)

The const here isn't needed or helpful.

> +        bool m_fetchedViaSpdy;

Even data boolean members work well with the "class-name <xxx>" naming scheme, so I would call this m_wasFetchedViaSpdy.

> +        Enabled only in Chromium.

Not true any more. And not really important to add in a Chromium directory ChangeLog.

> +        * src/WebURLResponse.cpp:
> +        (WebKit::WebURLResponse::isSpdy):
> +        (WebKit::WebURLResponse::setIsSpdy):

Names here out of date.

review- because of this round of nitpicks -- seems generally good

[Mike Belshe]

Created attachment 46007 [details]
Update with Darin A's comments.

Thanks Darin A. - comments addressed; sorry for my lameness.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46007 [details] without any errors.

[WebKit Commit Bot]

Comment on attachment 46007 [details]
Update with Darin A's comments.

Rejecting patch 46007 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/committers.py by adding yourself to the file (no review needed).  Due to bug 30084 the commit-queue will require a restart after your change.  Please contact [e-mail] to request a commit-queue restart.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 46007 [details]
Update with Darin A's comments.

Clearing flags on attachment: 46007

Committed r52898: <http://trac.webkit.org/changeset/52898>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Fisher (:fishd, Google)]

> If the name is SPDY, the we would prefer to have that in the identifiers, not
> Spdy.

It seems like this change wasn't applied in the final patch.  Oversight?

[Mike Belshe]

(In reply to comment #23)
> > If the name is SPDY, the we would prefer to have that in the identifiers, not
> > Spdy.
> 
> It seems like this change wasn't applied in the final patch.  Oversight?

Sorry- forgot to comment on that.  In the rest of chromium, we're going with Spdy, not SPDY, to match our use of Http, not HTTP.  So I was making this match.

[Mark Rowe (bdash)]

(In reply to comment #24)
> Sorry- forgot to comment on that.  In the rest of chromium, we're going with
> Spdy, not SPDY, to match our use of Http, not HTTP.  So I was making this
> match.

Surely code in WebCore should be following WebKit coding conventions.

[Mark Rowe (bdash)]

I also see no mention of a reviewer in <http://trac.webkit.org/changeset/52898>.  What’s up with that?

[Mike Belshe]

(In reply to comment #25)
> (In reply to comment #24)
> > Sorry- forgot to comment on that.  In the rest of chromium, we're going with
> > Spdy, not SPDY, to match our use of Http, not HTTP.  So I was making this
> > match.
> 
> Surely code in WebCore should be following WebKit coding conventions.

WebCore is inconsistent with this today; let me know if a "correct" casing is documented somewhere.  Hopefully this is not a significant issue.

Some examples from other code pre-existing in WebCore:
WebCore/platform/KURL.h:   procotolInHTTPFamily()
WebCore/dom/Document.h:    processHttpEquiv()
Qt port:                   isHttpOnly() and ignoreHttpError()
etc.

[Mike Belshe]

(In reply to comment #26)
> I also see no mention of a reviewer in
> <http://trac.webkit.org/changeset/52898>.  What’s up with that?

Sorry, my mistake.  So when people do code reviews, you first submit the review, then mark it review:?.  At this point, you don't know who the reviewer is going to be, so you leave it blank.  Later, after someone reviews it, you create a whole new patch changing the ChangeLog entry and upload that?  Sorry for the mistake.

[Darin Adler]

(In reply to comment #27)
> WebCore/platform/KURL.h:   procotolInHTTPFamily()

New code, represents our project style. This *is* the WebKit style.

> WebCore/dom/Document.h:    processHttpEquiv()

Very old code, dates back to the KHTML days.'

> Qt port:                   isHttpOnly() and ignoreHttpError()

Non-cross-port code often receives less-close scrutiny.

The WebKit style is to keep acronyms capitalized. This is explicit in <http://webkit.org/coding/coding-style.html>.

[Darin Adler]

(In reply to comment #28)
> Sorry, my mistake.  So when people do code reviews, you first submit the
> review, then mark it review:?.  At this point, you don't know who the reviewer
> is going to be, so you leave it blank.

You leave it in exactly this format.

    Reviewed by NOBODY (OOPS!).

This helps if you forget, because if you try to commit without changing that line the pre-commit hook will prevent you from checking in.

> Later, after someone reviews it, you
> create a whole new patch changing the ChangeLog entry and upload that?

No.

The committer is responsible for putting in the reviewer name. When the committer is a real live person there is some flexibility about what format the Reviewed by line is in. When the committer is the commit-queue script then it has to be exactly the correct format.

[Mike Belshe]

(In reply to comment #29)
> (In reply to comment #27)
> > WebCore/platform/KURL.h:   procotolInHTTPFamily()
> 
> New code, represents our project style. This *is* the WebKit style.
> 
> > WebCore/dom/Document.h:    processHttpEquiv()
> 
> Very old code, dates back to the KHTML days.'
> 
> > Qt port:                   isHttpOnly() and ignoreHttpError()
> 
> Non-cross-port code often receives less-close scrutiny.
> 
> The WebKit style is to keep acronyms capitalized. This is explicit in
> <http://webkit.org/coding/coding-style.html>.

I'll update.

[Mike Belshe]

Reopening to fix style issues.

[Mike Belshe]

On second thought, I better file a new bug.  I'm pretty sure someone will yell at me if I try to attach a style patch onto this bug.

[Adam Barth]

(In reply to comment #33)
> On second thought, I better file a new bug.  I'm pretty sure someone will yell at me if I try to attach a style patch onto this bug.

Thanks!  :)