Summary: | htmlediting.cpp needs more utility functions to fix the bug 26816 | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||||||||||
Component: | HTML Editing | Assignee: | Ryosuke Niwa <rniwa> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Enhancement | CC: | eric, jparent, justin.garcia | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||
Bug Blocks: | 26816 | ||||||||||||||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2009-07-07 12:48:37 PDT
Created attachment 32389 [details] fixes the bug This patch supplies utility functions to resolve the bug 26816. Created attachment 32391 [details]
removed tab from the patch
Lots of style nits. :( Comment on attachment 32391 [details] removed tab from the patch I'm not a real reviewer, but I saw a couple style violations. See below for details. Also, I know it isn't the style of these files, but could you add function level descriptions for the new functions you are adding? Particularly for functions that are designed to be re-usable utilities, documentation will really help others use them in the future, rather than rolling their own. > +PassRefPtr<Range> createRange(PassRefPtr<Document> document, const VisiblePosition& start, const VisiblePosition& end, ExceptionCode& ec) > +{ > + ec=0; > + RefPtr<Range> selectedRange = Range::create(document); > + selectedRange->setStart( start.deepEquivalent().containerNode(), start.deepEquivalent().computeOffsetInContainerNode(), ec); Style, here and in a few other places in this file: Do not place spaces between a function and its parentheses, or between a parenthesis and its content. > +PassRefPtr<Range> exntendRangeToWrappingNodes(PassRefPtr<Range> rangeToExtend, PassRefPtr<Range> maximumRange, Node* rootNode) Spelling: extend, not exntend > +{ > + ASSERT(rangeToExtend); > + ASSERT(maximumRange); > + > + ExceptionCode ec=0; Style: Here, and in other places in this file. Do place spaces around binary and ternary operators. > Index: WebCore/editing/htmlediting.h > =================================================================== > --- WebCore/editing/htmlediting.h (revision 45564) > +++ WebCore/editing/htmlediting.h (working copy) > @@ -28,6 +28,7 @@ > > #include <wtf/Forward.h> > #include "HTMLNames.h" > +#include "CompositeEditCommand.h" > > namespace WebCore { > > @@ -61,6 +62,7 @@ Position previousVisuallyDistinctCandida > bool isEditablePosition(const Position&); > bool isRichlyEditablePosition(const Position&); > Element* editableRootForPosition(const Position&); > + Element* unsplittableElementForPosition(const Position& p); Style: spacing Created attachment 32394 [details]
fixed the style violations
I removed getVisibleRangeOfNode and added visiblePositionBeforeNode and visiblePositionAfterNode. This would clean up the rest of my patch a little bit.
Created attachment 32395 [details]
updated ChangeLog
Comment on attachment 32395 [details] updated ChangeLog > Index: WebCore/editing/htmlediting.cpp > =================================================================== > --- WebCore/editing/htmlediting.cpp (revision 45564) > +++ WebCore/editing/htmlediting.cpp (working copy) > @@ -207,6 +207,15 @@ Element* editableRootForPosition(const P > return node->rootEditableElement(); > } > > +// Find the unsplittable element enclosing the specified position. > +// The returned element is either table cell or root editable node. Sorry I wasn't more clear before. I asked for function level comments because it wasn't clear to me from the function names what they should/should not be used for. These comments only repeat the function names, which isn't much more useful. For example, what does it mean to be an unsplittable element? > +Element* unsplittableElementForPosition(const Position& p) > +{ > + Element* enclosingCell = static_cast<Element*>(enclosingNodeOfType(p, &isTableCell)); > + > + return enclosingCell ? enclosingCell : editableRootForPosition(p); > +} This isn't quite right - what if the editableRoot is inside of the td? Created attachment 32400 [details]
added more comments to each function
Created attachment 32402 [details]
removed the change to CompositeEditCommand.cpp
Comment on attachment 32402 [details]
removed the change to CompositeEditCommand.cpp
Style:
63 short comparePoint(Node* refNode, int offset, ExceptionCode& ec) const;
I would have used early return:
215 Element* enclosingCell = static_cast<Element*>(enclosingNodeOfType(p, &isTableCell, true));
216 // Since enclosingNodeOfType won't search beyond the highest root editable node,
217 // this code works even if the closest table cell was outside of the root editable node.
218 return enclosingCell ? enclosingCell : editableRootForPosition(p);
This could assert ASSERT(node->parentNode()):
592 VisiblePosition visiblePositionBeforeNode(const Node* node)
Please add a FIXME: to the other createRange:
612 PassRefPtr<Range> createRange(PassRefPtr<Document> document, const VisiblePosition& start, const VisiblePosition& end, ExceptionCode& ec)
PassRefPtr seems the wrong type for these args:
627 PassRefPtr<Range> extendRangeToWrappingNodes(PassRefPtr<Range> rangeToExtend, PassRefPtr<Range> maximumRange, Node* rootNode)
Explicit visiblePosition construction is not needed here:
1076 return visiblePositionBeforeNode(node) == VisiblePosition(selectedRange.startPosition())
(but it also doesn't hurt)
Style:
65 Element* unsplittableElementForPosition(const Position& p);
78 PassRefPtr<Range> createRange(PassRefPtr<Document> document, const VisiblePosition& start, const VisiblePosition& end, ExceptionCode& ec);
145 bool isNodeVisiblyContainedWithin(Node* node, const Range& selectedRange);
Looks fine. Normally we don't add unused code, but I can see how this will help make your future patches smaller.
Please re-submit a fixed version, and then we might wait to land this until the other patch which depends on these changes is approved to.
Created attachment 32484 [details]
resubmission
Comment on attachment 32484 [details]
resubmission
Looks fine.
I think the intent of the old comments (that something needs fixing) is lost:
// FIXME: This should ASSERT(node->parentNode())
582 ASSERT(node);
583 // FIXME: adding ASSERT(node->parentNode()); results in editing/deleting/delete-ligature-001.html crashing
569584
No need to be const:
595 VisiblePosition visiblePositionBeforeNode(const Node* node)
Generally we don't make RefCounted objects const, ever. Because well... they're refcounted. :) You can't ref them w/ them being const.
I probably would have named the argument "range" and the returned value "extendRange" but this is fine too:
632 PassRefPtr<Range> extendRangeToWrappingNodes(PassRefPtr<Range> rangeToExtend, const Range* maximumRange, const Node* rootNode)
Thanks.!
Created attachment 32973 [details]
re-resubmission
Comment on attachment 32973 [details]
re-resubmission
Why is this needed?
#include "HTMLNames.h"
+#include "CompositeEditCommand.h"
Seems wrong.
Created attachment 32977 [details]
9th submission
Comment on attachment 32977 [details]
9th submission
Generally we sort headers in alphabetical order, but this is fine to land as-is.
Landed in http://trac.webkit.org/changeset/46063 |