Bug 95500 - [chromium] Make RenderPass ids hold both generating layer id and an index
Summary: [chromium] Make RenderPass ids hold both generating layer id and an index
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dana Jansens
URL:
Keywords:
Depends on: 96073
Blocks: 94145
  Show dependency treegraph
 
Reported: 2012-08-30 14:41 PDT by Dana Jansens
Modified: 2012-09-07 12:37 PDT (History)
9 users (show)

See Also:


Attachments
Patch (38.88 KB, patch)
2012-08-30 14:45 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (38.90 KB, patch)
2012-08-30 14:46 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (38.88 KB, patch)
2012-08-30 14:50 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (39.59 KB, patch)
2012-08-30 16:16 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (41.16 KB, patch)
2012-09-05 18:29 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (42.61 KB, patch)
2012-09-06 09:08 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch for landing (43.88 KB, patch)
2012-09-06 13:14 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch for landing (43.88 KB, patch)
2012-09-07 11:14 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dana Jansens 2012-08-30 14:41:28 PDT
[chromium] Make RenderPass ids hold both generating layer id and an index
Comment 1 Dana Jansens 2012-08-30 14:45:10 PDT
Created attachment 161561 [details]
Patch
Comment 2 Dana Jansens 2012-08-30 14:46:50 PDT
Created attachment 161562 [details]
Patch
Comment 3 Dana Jansens 2012-08-30 14:50:02 PDT
Created attachment 161563 [details]
Patch
Comment 4 Dana Jansens 2012-08-30 16:16:42 PDT
Created attachment 161578 [details]
Patch

fix debug build
Comment 5 Adrienne Walker 2012-09-05 14:04:31 PDT
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.
Comment 6 Dana Jansens 2012-09-05 16:48:25 PDT
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?
Comment 7 Adrienne Walker 2012-09-05 16:51:09 PDT
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.
Comment 8 Dana Jansens 2012-09-05 16:56:23 PDT
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.
Comment 9 Dana Jansens 2012-09-05 18:29:36 PDT
Created attachment 162388 [details]
Patch

Not using std::pair anymore. Indented the struct.
Comment 10 Peter Beverloo (cr-android ews) 2012-09-05 22:26:23 PDT
Comment on attachment 162388 [details]
Patch

Attachment 162388 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13768249
Comment 11 WebKit Review Bot 2012-09-06 06:03:59 PDT
Comment on attachment 162388 [details]
Patch

Attachment 162388 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13771330
Comment 12 Adrienne Walker 2012-09-06 08:36:20 PDT
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.
Comment 13 Dana Jansens 2012-09-06 08:39:36 PDT
(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.
Comment 14 Dana Jansens 2012-09-06 09:08:40 PDT
Created attachment 162520 [details]
Patch

rebased and using PairHash
Comment 15 Adrienne Walker 2012-09-06 09:13:19 PDT
Comment on attachment 162520 [details]
Patch

R=me.
Comment 16 Dana Jansens 2012-09-06 13:14:35 PDT
Created attachment 162564 [details]
Patch for landing
Comment 17 WebKit Review Bot 2012-09-06 18:33:33 PDT
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
Comment 18 WebKit Review Bot 2012-09-06 21:48:58 PDT
Comment on attachment 162564 [details]
Patch for landing

Clearing flags on attachment: 162564

Committed r127822: <http://trac.webkit.org/changeset/127822>
Comment 19 WebKit Review Bot 2012-09-06 21:49:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 WebKit Review Bot 2012-09-06 23:10:10 PDT
Re-opened since this is blocked by 96073
Comment 21 Dana Jansens 2012-09-07 11:14:55 PDT
Created attachment 162827 [details]
Patch for landing
Comment 22 WebKit Review Bot 2012-09-07 12:37:07 PDT
Comment on attachment 162827 [details]
Patch for landing

Clearing flags on attachment: 162827

Committed r127907: <http://trac.webkit.org/changeset/127907>
Comment 23 WebKit Review Bot 2012-09-07 12:37:11 PDT
All reviewed patches have been landed.  Closing bug.