Summary: | Web Inspector: WebKit2: need API to when the Inspector is in the front | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Timothy Hatcher <timothy> | ||||||
Component: | Web Inspector (Deprecated) | Assignee: | Timothy Hatcher <timothy> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | apavlov, bweinstein, eric, joepeck, keishi, loislo, menard, pfeldman, pmuellr, rik, webkit.review.bot, yurys, zoltan | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Timothy Hatcher
2012-02-27 02:03:58 PST
Created attachment 128990 [details]
Proposed Change
Attachment 128990 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5]
Total errors found: 1 in 11 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 128990 [details] Proposed Change View in context: https://bugs.webkit.org/attachment.cgi?id=128990&action=review > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:126 > + [menuItem setTitle:WEB_UI_STRING("Hide Web Inspector", "title for Hide Web Inspector menu item")]; Don’t you also need to handle the “Show Web Inspector” case here? > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:257 > + return m_isVisible && [m_inspectorView.get().window isMainWindow]; It would be good to reference a bug number in this FIXME. Comment on attachment 128990 [details] Proposed Change View in context: https://bugs.webkit.org/attachment.cgi?id=128990&action=review > Source/WebKit2/UIProcess/win/WebInspectorProxyWin.cpp:245 > + notImplemented(); It looks like you need NotImplemented.h for the Windows build. (In reply to comment #3) > (From update of attachment 128990 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=128990&action=review > > > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:126 > > + [menuItem setTitle:WEB_UI_STRING("Hide Web Inspector", "title for Hide Web Inspector menu item")]; > > Don’t you also need to handle the “Show Web Inspector” case here? No, sicne if the Web Inspector's window delegate is handleing the validation we know it is front. I'll add a comment. > > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:257 > > + return m_isVisible && [m_inspectorView.get().window isMainWindow]; > > It would be good to reference a bug number in this FIXME. Will do. I'll fix those and the Windows build and upload a new patch. Created attachment 129049 [details]
Proposed Change (round 2)
Attachment 129049 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5]
Total errors found: 1 in 11 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 129049 [details] Proposed Change (round 2) View in context: https://bugs.webkit.org/attachment.cgi?id=129049&action=review > Source/WebCore/ChangeLog:8 > + Needs a bug number here. > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:95 > +} Is there something preventing us from updating the method name to showOrHideWebInspector: or something along those lines? > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:254 > + // FIXME: this will not bring a background tab in Safari to the front, only its window. <rdar://problem/10937688> Our normal style is "FIXME 10937688:" or "FIXME <rdar://problem/10937688>". > Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:260 > + // FIXME: this will not return false for a background tab in Safari, only a background window. <rdar://problem/10937688> Same here. Attachment 129049 [details] was posted by a committer and has review+, assigning to Timothy Hatcher for commit.
This landed in r109002. |