Summary: | AX: debugging attributes for text markers | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Doug Russell <d_russell> | ||||||||||
Component: | Accessibility | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, cfleizach, cgarcia, clopez, commit-queue, dmazzoni, esprehn+autocc, hyungwook.lee, jcraig, jdiggs, kangil.han, mario, mrobinson, ossy, samuel_white, webkit-bug-importer, zan | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=145608 | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 145596 | ||||||||||||
Attachments: |
|
Description
Doug Russell
2015-05-21 14:21:13 PDT
Created attachment 253542 [details]
patch
Comment on attachment 253542 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=253542&action=review thanks > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3796 > + if ([attribute isEqualToString:@"AXTextMarkerDebugDescription"]) { can you put these into a separate method completely and can you put the logic in an ObjC method so that these can be called from within the debugger itself > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3797 > + VisiblePosition visiblePosition = [self visiblePositionForTextMarker:(textMarker)]; i don't think the parens are necessary here (textMarker) > Source/WebCore/dom/Text.cpp:231 > + result.appendLiteral("count="); this looks like it should be length instead of count also change log needed Created attachment 253692 [details]
patch
Comment on attachment 253692 [details]
patch
Doesn't seem like any review comments were addressed
(In reply to comment #6) > Comment on attachment 253692 [details] > patch > > Doesn't seem like any review comments were addressed Sorry, uploaded the old patch. Attaching the right one now. Created attachment 253714 [details]
patch
Comment on attachment 253714 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=253714&action=review > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3495 > + VisiblePosition visiblePosition = [self visiblePositionForTextMarker:textMarker]; > + const int FormatBufferSize = 1024; > + char format[FormatBufferSize]; > + visiblePosition.formatForDebugger(format, FormatBufferSize); > + return (NSString *)String(format, FormatBufferSize); I would write this differently: char description[1024]; [self visiblePositionForTextMarker:textMarker](description, sizeof(description)); return [NSString stringWithUTF8String:description]; There’s no reason for the local variables or to involve WTF::String. > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3506 > + const int FormatBufferSize = 2048; > + char format[FormatBufferSize]; > + formatForDebugger(visiblePositionRange, format, FormatBufferSize); > + return (NSString *)String(format, FormatBufferSize); I would write this differently: char description[2048]; formatForDebugger(visiblePositionRange, description, sizeof(description)); return [NSString stringWithUTF8String:description]; > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3515 > + node->showNode(""); This should just be: node->showNode(); No reason to pass "" explicitly. > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3541 > + strncpy(buffer, result.toString().utf8().data(), length - 1); Even though the other format for debugger functions incorrectly do this, you should not do it. The good way to do it in Cocoa-specific code is: strlcpy(buffer, result.toString().utf8().data(), length); As you can see on the man page for strncpy, it doesn’t do the right thing unless you write an additional line of code to NUL terminate the string when it overflows the buffer size. This also should be fixed in the other places in WebKit that do this incorrectly, but please don’t introduce a new one. > Source/WebCore/dom/Text.cpp:234 > + result.appendLiteral("; "); > + result.appendLiteral("value=\""); Seems like those could be a single call to appendLiteral. > Source/WebCore/dom/Text.cpp:236 > + result.appendLiteral("\""); This could be append('"') instead, slightly more efficient. Created attachment 254126 [details]
patch
updates from review
Comment on attachment 254126 [details] patch Rejecting attachment 254126 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 254126, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.appspot.com/results/5969369683197952 Comment on attachment 254126 [details] patch Clearing flags on attachment: 254126 Committed r185137: <http://trac.webkit.org/changeset/185137> All reviewed patches have been landed. Closing bug. (In reply to comment #12) > Comment on attachment 254126 [details] > patch > > Clearing flags on attachment: 254126 > > Committed r185137: <http://trac.webkit.org/changeset/185137> It broke the debug GTK build: ../../Source/WebCore/dom/Text.cpp:238:60: error: ‘strlcpy’ was not declared in this scope strlcpy(buffer, result.toString().utf8().data(), length); $man strncpy "strlcpy() is not present in glibc and is not standardized by POSIX, but is available on Linux via the libbsd library." I just reported this issue, but don't have any time to try to fix it myself. cc-ing GTK maintainters. (In reply to comment #14) > (In reply to comment #12) > > Comment on attachment 254126 [details] > > patch > > > > Clearing flags on attachment: 254126 > > > > Committed r185137: <http://trac.webkit.org/changeset/185137> > > It broke the debug GTK build: > ../../Source/WebCore/dom/Text.cpp:238:60: error: ‘strlcpy’ was not declared > in this scope > strlcpy(buffer, result.toString().utf8().data(), length); > > $man strncpy > "strlcpy() is not present in glibc and is not standardized by POSIX, > but is available on Linux via the libbsd library." > > I just reported this issue, but don't have any time > to try to fix it myself. cc-ing GTK maintainters. EFL port has same build break too. I upload a fix to Bug 145596. |