Bug 75222 - Use unique_ptr for FillLayer::m_next
Summary: Use unique_ptr for FillLayer::m_next
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks: 41321
  Show dependency treegraph
 
Reported: 2011-12-25 21:42 PST by Darin Adler
Modified: 2014-04-13 21:28 PDT (History)
9 users (show)

See Also:


Attachments
Patch (12.05 KB, patch)
2011-12-25 21:45 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (12.73 KB, patch)
2011-12-25 23:10 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (25.71 KB, patch)
2014-04-12 22:58 PDT, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2011-12-25 21:42:55 PST
Use OwnPtr for FillLayer::m_next
Comment 1 Darin Adler 2011-12-25 21:45:44 PST
Created attachment 120525 [details]
Patch
Comment 2 mitz 2011-12-25 21:54:40 PST
Comment on attachment 120525 [details]
Patch

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

r- because of a logic mistake I spotted in imagesAreLoaded().

> Source/WebCore/rendering/style/FillLayer.cpp:80
>  FillLayer::~FillLayer()
>  {
> -    delete m_next;
>  }

Wouldn’t the compiler generate this destructor if you omitted it (and didn’t declare it)?

> Source/WebCore/rendering/style/FillLayer.cpp:255
> -            return false;
> +    for (const FillLayer* layer = this; layer; layer = layer->m_next.get()) {
> +        if (layer->m_image && !layer->m_image->isLoaded())
> +            return true;

Should return false here.
Comment 3 Darin Adler 2011-12-25 22:24:31 PST
Comment on attachment 120525 [details]
Patch

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

>> Source/WebCore/rendering/style/FillLayer.cpp:80
>>  }
> 
> Wouldn’t the compiler generate this destructor if you omitted it (and didn’t declare it)?

It would, but further, it would probably be inlined each place a FillLayer was destroyed. But I’ll do as you imply I should.

>> Source/WebCore/rendering/style/FillLayer.cpp:255
>> +            return true;
> 
> Should return false here.

Oops!
Comment 4 Darin Adler 2011-12-25 23:10:25 PST
Created attachment 120529 [details]
Patch
Comment 5 WebKit Review Bot 2012-05-24 10:45:36 PDT
Comment on attachment 120529 [details]
Patch

Rejecting attachment 120529 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ss/CSSStyleApplyProperty.cpp.rej
patching file Source/WebCore/rendering/style/FillLayer.cpp
Hunk #2 succeeded at 40 with fuzz 1 (offset 15 lines).
Hunk #3 succeeded at 65 (offset 15 lines).
Hunk #4 succeeded at 90 (offset 15 lines).
Hunk #5 succeeded at 239 (offset 15 lines).
patching file Source/WebCore/rendering/style/FillLayer.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Dan Bernst..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12798232
Comment 6 Darin Adler 2012-05-24 11:07:29 PDT
Guess I’ll have to rebase this myself. Not in front of a computer with a source tree at the moment.
Comment 7 Alexey Proskuryakov 2013-03-16 22:56:32 PDT
This hasn't been landed yet.
Comment 8 Darin Adler 2014-04-12 22:58:13 PDT
Created attachment 229219 [details]
Patch
Comment 9 WebKit Commit Bot 2014-04-12 23:06:54 PDT
Attachment 229219 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/style/FillLayer.cpp:76:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Darin Adler 2014-04-13 21:28:49 PDT
Committed r167208: <http://trac.webkit.org/changeset/167208>