Bug 97871 - RenderImage should not use shadowAncestorNode
Summary: RenderImage should not use shadowAncestorNode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shinya Kawanaka
URL:
Keywords:
Depends on:
Blocks: 91821
  Show dependency tree / graph
 
Reported: 2012-09-27 23:58 PDT by Shinya Kawanaka
Modified: 2013-01-06 22:04 PST (History)
3 users (show)

See Also:

Attachments
Patch (1.51 KB, patch)
2012-09-28 00:01 PDT, Shinya Kawanaka 		no flags Details | Formatted Diff | Diff
Patch for landing (1.54 KB, patch)
2012-10-08 21:42 PDT, Shinya Kawanaka 		no flags Details | Formatted Diff | Diff
Patch for landing (1.56 KB, patch)
2013-01-06 21:06 PST, Shinya Kawanaka 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2012-09-27 23:58:24 PDT 
Since node()->shadowAncestorNode() does not return NULL, this if statement will never fail.
This should be shadowHost().
Comment 1 Shinya Kawanaka 2012-09-28 00:01:26 PDT 
Created attachment 166158 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-09-28 09:01:40 PDT 
I'm confused.  How is this covered by existing tests?  Is it a behavior change?
Comment 3 Dimitri Glazkov (Google) 2012-09-28 09:15:43 PDT 
(In reply to comment #2)
> I'm confused.  How is this covered by existing tests?  Is it a behavior change?

It's not, it's a fix in the logic. shadowAncestorNode is a weird cousin of shadowRoot. We're trying to get rid of this weird cousin and in this particular case shadowRoot and shadowAncestorNode can be used interchangeably.

So, the ChangeLog should say: No change in logic, refactoring.
Comment 4 Dimitri Glazkov (Google) 2012-09-28 09:16:00 PDT 
Comment on attachment 166158 [details]
Patch

Please fix the ChangeLog before landing.
Comment 5 Shinya Kawanaka 2012-10-08 21:42:08 PDT 
Created attachment 167687 [details]
Patch for landing
Comment 6 WebKit Review Bot 2012-10-08 22:13:56 PDT 
Comment on attachment 167687 [details]
Patch for landing

Rejecting attachment 167687 [details] from commit-queue.

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

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14214605
Comment 7 Shinya Kawanaka 2013-01-06 21:06:47 PST 
Created attachment 181472 [details]
Patch for landing
Comment 8 Shinya Kawanaka 2013-01-06 21:07:48 PST 
I didn't notice this patch was not committed yet.
rebase on ToT, and cq+.
Comment 9 WebKit Review Bot 2013-01-06 22:04:37 PST 
Comment on attachment 181472 [details]
Patch for landing

Clearing flags on attachment: 181472

Committed r138922: <http://trac.webkit.org/changeset/138922>
Comment 10 WebKit Review Bot 2013-01-06 22:04:41 PST 
All reviewed patches have been landed.  Closing bug.