Bug 95500 – [chromium] Make RenderPass ids hold both generating layer id and an index

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[chromium] Make RenderPass ids hold both generating layer id and an index

[Dana Jansens]

Created attachment 161561 [details]
Patch

[Dana Jansens]

Created attachment 161562 [details]
Patch

[Dana Jansens]

Created attachment 161563 [details]
Patch

[Dana Jansens]

Created attachment 161578 [details]
Patch

fix debug build

[Adrienne Walker]

Comment on attachment 161578 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161578&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:63
> +    typedef std::pair<int, int> Id;

Bah, std::pair.  How about a struct with decent, upstanding names?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:119
> +static const bool emptyValueIsZero = false;
> +static const bool needsDestruction = false;
> +static WebCore::CCRenderPass::Id emptyValue() { return std::make_pair(0, 0); }
> +static void constructDeletedValue(WebCore::CCRenderPass::Id& slot) { slot = std::make_pair(-1, -1); }
> +static bool isDeletedValue(WebCore::CCRenderPass::Id value) { return value.first == -1 && value.second == -1; }

Indent these.

[Dana Jansens]

Comment on attachment 161578 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161578&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:63
>> +    typedef std::pair<int, int> Id;
> 
> Bah, std::pair.  How about a struct with decent, upstanding names?

Haha ok, I did that at first but then started implementing things like operator== and thought a pair would be so clever.

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:119
>> +static bool isDeletedValue(WebCore::CCRenderPass::Id value) { return value.first == -1 && value.second == -1; }
> 
> Indent these.

"The contents of an outermost namespace block (and any nested namespaces with the same scope) should not be indented. The contents of other nested namespaces should be indented."

Just checking, but this is an outermost block, isn't it? Is this an exception?

[Adrienne Walker]

Comment on attachment 161578 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161578&action=review

>>> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:119
>>> +static bool isDeletedValue(WebCore::CCRenderPass::Id value) { return value.first == -1 && value.second == -1; }
>> 
>> Indent these.
> 
> "The contents of an outermost namespace block (and any nested namespaces with the same scope) should not be indented. The contents of other nested namespaces should be indented."
> 
> Just checking, but this is an outermost block, isn't it? Is this an exception?

It's the contents of a struct, not the contents of a namespace.

[Dana Jansens]

Comment on attachment 161578 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161578&action=review

>>>> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:119
>>>> +static bool isDeletedValue(WebCore::CCRenderPass::Id value) { return value.first == -1 && value.second == -1; }
>>> 
>>> Indent these.
>> 
>> "The contents of an outermost namespace block (and any nested namespaces with the same scope) should not be indented. The contents of other nested namespaces should be indented."
>> 
>> Just checking, but this is an outermost block, isn't it? Is this an exception?
> 
> It's the contents of a struct, not the contents of a namespace.

Oh... blush. Sorry, thanks.

[Dana Jansens]

Created attachment 162388 [details]
Patch

Not using std::pair anymore. Indented the struct.

[Peter Beverloo (cr-android ews)]

Comment on attachment 162388 [details]
Patch

Attachment 162388 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13768249

[WebKit Review Bot]

Comment on attachment 162388 [details]
Patch

Attachment 162388 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13771330

[Adrienne Walker]

View in context: https://bugs.webkit.org/attachment.cgi?id=162388&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:136
> +template<> struct IntHash<WebCore::CCRenderPass::Id> {

Why does CCRenderPass::Id need better hashing than IntHash<std::pair<int, int> >?  You should just use the default and then make the default better in all cases.

[Dana Jansens]

(In reply to comment #12)
> View in context: https://bugs.webkit.org/attachment.cgi?id=162388&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:136
> > +template<> struct IntHash<WebCore::CCRenderPass::Id> {
> 
> Why does CCRenderPass::Id need better hashing than IntHash<std::pair<int, int> >?  You should just use the default and then make the default better in all cases.

Ya, cuz everything needs better than that. I am planning to upload a patch to fix std::pair as well, but felt wrong to use the bad function in new code. I guess I can though.

[Dana Jansens]

Created attachment 162520 [details]
Patch

rebased and using PairHash

[Adrienne Walker]

Comment on attachment 162520 [details]
Patch

R=me.

[Dana Jansens]

Created attachment 162564 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 162564 [details]
Patch for landing

Rejecting attachment 162564 [details] from commit-queue.

New failing tests:
http/tests/cache/subresource-expiration-1.html
http/tests/cache/stopped-revalidation.html
Full output: http://queues.webkit.org/results/13786213

[WebKit Review Bot]

Comment on attachment 162564 [details]
Patch for landing

Clearing flags on attachment: 162564

Committed r127822: <http://trac.webkit.org/changeset/127822>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 96073

[Dana Jansens]

Created attachment 162827 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 162827 [details]
Patch for landing

Clearing flags on attachment: 162827

Committed r127907: <http://trac.webkit.org/changeset/127907>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.