Summary: | MarkupAccumulator::appendStartMarkup should be broken down into pieces | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||
Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Enhancement | CC: | abarth, darin, eric, ojan, tkent, tony, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | 44229, 44318 | ||||||||||
Bug Blocks: | 44831 | ||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2010-08-19 12:50:22 PDT
Created attachment 64897 [details]
cleanup
(In reply to comment #1) > Created an attachment (id=64897) [details] > cleanup Whole CharEntity business is very ugly. It'll be greatly appreciated if anyone can tell me a better way of defining an array of CharEntity's. Attachment 64897 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/editing/markup.cpp:636: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #3) > Attachment 64897 [details] did not pass style-queue: > > Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 > WebCore/editing/markup.cpp:636: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] > Total errors found: 1 in 2 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. This is a willful violation of the style guideline because the patch becomes unintelligible (svn diff tries to replace appendStartMarkup by appendElement, interleaving bits of pieces of both functions). Comment on attachment 64897 [details]
cleanup
Thanks for working on this code. It's blowing my mind too much to review properly.
WebCore/ChangeLog:14
+ No new tests. (OOPS!)
This can't land with OOPS
If you're going to do a lot of work on markup.cpp, please consider cleaning up all the style in a first pass. Created attachment 64901 [details] fixed "No new tests. (OOPS!)" (In reply to comment #5) > (From update of attachment 64897 [details]) > Thanks for working on this code. It's blowing my mind too much to review properly. > > WebCore/ChangeLog:14 > + No new tests. (OOPS!) > This can't land with OOPS Sorry, I meant to change that line to "No new tests are added since this is a cleanup." Fixed. (In reply to comment #6) > If you're going to do a lot of work on markup.cpp, please consider cleaning up all the style in a first pass. Which function / part of code are you referring to? > > If you're going to do a lot of work on markup.cpp, please consider cleaning up all the style in a first pass.
>
> Which function / part of code are you referring to?
Oh, I just noticed that the style bot was red. I'd just work through the file until check-webkit-style passes. That makes future changes easier to review because we don't have to worry about existing bad style.
Attachment 64901 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/editing/markup.cpp:636: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 64993 [details]
cleanup
Removed escapeEntityReferences because that can be done in a separate patch
Comment on attachment 64993 [details]
cleanup
WebCore/editing/markup.cpp:490
+ const QualifiedName* parentName = 0;
I'm not sure I understand why we use a pointer here. It seems using a QualifiedName local would work just as well. I can't imaging its any slower since QualifiedName it itself just a RefPtr.
WebCore/editing/markup.cpp:489
+ bool documentIsHTML = text->document()->isHTMLDocument();
Seems this is only used in one place in this function.
WebCore/editing/markup.cpp:580
+ append(out, attribute->name().localName());
is localName all caps or something? Confused why this is needed.
WebCore/editing/markup.cpp:587
+ // We don't want to complete file:/// URLs because it may contain sensitive information
Are there other file-like urls that could get us into trouble?
WebCore/editing/markup.cpp:604
+ RefPtr<CSSMutableStyleDeclaration> style = static_cast<HTMLElement*>(element)->getInlineStyleDecl()->copy();
Wow, this block wants to be broken up again too. :)
r+ since you're just moving code. But you might consider my comments for a follow-up patch. I suspect we also need more testing here, but that can come when we start fixing stuff here. :)
Thank you SO MUCH for working on this!
(In reply to comment #11) > (From update of attachment 64993 [details]) > WebCore/editing/markup.cpp:490 > + const QualifiedName* parentName = 0; > I'm not sure I understand why we use a pointer here. It seems using a QualifiedName local would work just as well. I can't imaging its any slower since QualifiedName it itself just a RefPtr. Will ask on webkit-dev since QualifiedName's default constructor is hidden behind a flag. > WebCore/editing/markup.cpp:489 > + bool documentIsHTML = text->document()->isHTMLDocument(); > Seems this is only used in one place in this function. Will fix. > WebCore/editing/markup.cpp:580 > + append(out, attribute->name().localName()); > is localName all caps or something? Confused why this is needed. toString gives back namespace:localName and we want to avoid that if the document is not XML. > WebCore/editing/markup.cpp:587 > + // We don't want to complete file:/// URLs because it may contain sensitive information > Are there other file-like urls that could get us into trouble? I'm not sure. Probably need some security review here. > WebCore/editing/markup.cpp:604 > + RefPtr<CSSMutableStyleDeclaration> style = static_cast<HTMLElement*>(element)->getInlineStyleDecl()->copy(); > Wow, this block wants to be broken up again too. :) Yes. But I'm thinking of doing this whole style thing up front so that we don't have to special-case style attribute at the very end. > r+ since you're just moving code. But you might consider my comments for a follow-up patch. I suspect we also need more testing here, but that can come when we start fixing stuff here. :) Since this code is shared in copying & innerHTML, I'd suspect that the test coverage isn't so bad. Changing one or two lines will definitely result in failure of several tests but I wouldn't mind having more structured tests as well. > Thank you SO MUCH for working on this! My pleasure, thanks for the review. Committed r65854: <http://trac.webkit.org/changeset/65854> http://trac.webkit.org/changeset/65854 might have broken GTK Linux 32-bit Release The following changes are on the blame list: http://trac.webkit.org/changeset/65856 http://trac.webkit.org/changeset/65857 http://trac.webkit.org/changeset/65854 http://trac.webkit.org/changeset/65855 (In reply to comment #11) > (From update of attachment 64993 [details]) > WebCore/editing/markup.cpp:490 > + const QualifiedName* parentName = 0; > I'm not sure I understand why we use a pointer here. It seems using a QualifiedName local would work just as well. I can't imagine its any slower since QualifiedName it itself just a RefPtr. I think a pointer is a good way to handle an optional value. Also, a RefPtr is slower than a raw pointer. |