Bug 112072

Summary: Replace static_casts with to* functions - Part 2
Product: WebKit Reporter: Abhishek Arya <inferno>
Component: WebKit Misc.Assignee: Abhishek Arya <inferno>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, pdr, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 112097    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Abhishek Arya 2013-03-11 15:05:08 PDT
Replace static_casts with to* functions
Comment 1 Abhishek Arya 2013-03-11 15:08:23 PDT
Created attachment 192570 [details]
Patch
Comment 2 Philip Rogers 2013-03-11 15:29:09 PDT
Comment on attachment 192570 [details]
Patch

Can you add a bit more to the ChangeLog so people aren't confused when they see this in a few years? Just a small summary of what we're doing here.

LGTM otherwise.
Comment 3 Abhishek Arya 2013-03-11 15:35:50 PDT
Created attachment 192579 [details]
Patch
Comment 4 WebKit Review Bot 2013-03-11 19:01:25 PDT
Comment on attachment 192579 [details]
Patch

Clearing flags on attachment: 192579

Committed r145462: <http://trac.webkit.org/changeset/145462>
Comment 5 WebKit Review Bot 2013-03-11 19:01:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 WebKit Review Bot 2013-03-11 19:49:33 PDT
Re-opened since this is blocked by bug 112097
Comment 7 Abhishek Arya 2013-03-11 20:11:42 PDT
Created attachment 192629 [details]
Patch
Comment 8 Abhishek Arya 2013-03-11 22:47:27 PDT
Loads of toElement() crashes in WebCore::ApplyStyleCommand::applyBlockStyle

http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r145468%20(7365)/results.html
Comment 9 Ryosuke Niwa 2013-03-11 22:52:38 PDT
(In reply to comment #8)
> Loads of toElement() crashes in WebCore::ApplyStyleCommand::applyBlockStyle
> 
> http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r145468%20(7365)/results.html

That's a false positive. We should be using toContainerNode instead.
Comment 10 Ryosuke Niwa 2013-03-11 22:54:54 PDT
Comment on attachment 192629 [details]
Patch

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

> Source/WebCore/editing/ApplyStyleCommand.cpp:294
> -    startRange = TextIterator::rangeFromLocationAndLength(static_cast<Element*>(scope), startIndex, 0, true);
> -    endRange = TextIterator::rangeFromLocationAndLength(static_cast<Element*>(scope), endIndex, 0, true);
> +    startRange = TextIterator::rangeFromLocationAndLength(toElement(scope), startIndex, 0, true);
> +    endRange = TextIterator::rangeFromLocationAndLength(toElement(scope), endIndex, 0, true);

Please use toContainerNode here. scope can be Document.
Comment 11 Abhishek Arya 2013-03-11 22:58:06 PDT
(In reply to comment #10)
> (From update of attachment 192629 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192629&action=review
> 
> > Source/WebCore/editing/ApplyStyleCommand.cpp:294
> > -    startRange = TextIterator::rangeFromLocationAndLength(static_cast<Element*>(scope), startIndex, 0, true);
> > -    endRange = TextIterator::rangeFromLocationAndLength(static_cast<Element*>(scope), endIndex, 0, true);
> > +    startRange = TextIterator::rangeFromLocationAndLength(toElement(scope), startIndex, 0, true);
> > +    endRange = TextIterator::rangeFromLocationAndLength(toElement(scope), endIndex, 0, true);
> 
> Please use toContainerNode here. scope can be Document.

Thanks a lot Ryosuke for confirming. I don't understand why cr-linux-debug didnt shout at me :(.
Comment 12 Abhishek Arya 2013-03-11 22:58:41 PDT
Created attachment 192643 [details]
Patch
Comment 13 Ryosuke Niwa 2013-03-11 22:58:54 PDT
(In reply to comment #11)
>
> Thanks a lot Ryosuke for confirming. I don't understand why cr-linux-debug didnt shout at me :(.

cr-linux-debug doesn't run tests.
Comment 14 Build Bot 2013-03-12 09:10:37 PDT
Comment on attachment 192643 [details]
Patch

Attachment 192643 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17174047

New failing tests:
platform/mac/fast/speechsynthesis/speech-synthesis-speak.html
Comment 15 Abhishek Arya 2013-03-12 09:55:00 PDT
Lets me check these out locally, they might be real bugs.

Regressions: Unexpected crashes (2)
  editing/selection/selection-modify-crash.html [ Crash ]
  platform/mac/fast/speechsynthesis/speech-synthesis-speak.html [ Crash ]
Comment 16 Abhishek Arya 2013-03-12 10:16:32 PDT
Comment on attachment 192643 [details]
Patch

Nothing crashes locally on mac, going cq+.
Comment 17 WebKit Review Bot 2013-03-12 10:46:50 PDT
Comment on attachment 192643 [details]
Patch

Clearing flags on attachment: 192643

Committed r145562: <http://trac.webkit.org/changeset/145562>
Comment 18 WebKit Review Bot 2013-03-12 10:46:54 PDT
All reviewed patches have been landed.  Closing bug.