Bug 130709 - [iOS] Inspector View Indication Support
Summary: [iOS] Inspector View Indication Support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-03-24 19:44 PDT by Joseph Pecoraro
Modified: 2014-03-26 11:04 PDT (History)
5 users (show)

See Also:

Attachments
[PATCH] Proposed Fix (13.23 KB, patch)
2014-03-24 19:48 PDT, Joseph Pecoraro 		simon.fraser: review-
Details | Formatted Diff | Diff
[PATCH] Renamed methods and classes (13.43 KB, patch)
2014-03-25 12:11 PDT, Joseph Pecoraro 		simon.fraser: review-
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
[PATCH] More renames (20.92 KB, patch)
2014-03-25 13:26 PDT, Joseph Pecoraro 		simon.fraser: review+
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2014-03-24 19:44:40 PDT 
iOS view indication for the remote web inspector needs to be implemented.

On incoming Indicate RWI messages, we should show the typical blue highlight over the WKWebView.

<rdar://problem/16415035>
Comment 1 Joseph Pecoraro 2014-03-24 19:48:15 PDT 
Created attachment 227716 [details]
[PATCH] Proposed Fix
Comment 2 Simon Fraser (smfr) 2014-03-24 23:07:00 PDT 
Comment on attachment 227716 [details]
[PATCH] Proposed Fix

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1097
> +- (void)_startIndicatingView

"indicatingView" is ambiguous and not really good English. It should be read as -(UIView*)indicatingView, but I think you mean it as the view is "indicating the state of being inspected".

How about showShowingInspectionIndication/indicator? Or -(void)setIsShowingInspectionIndicator:(BOOL)?

> Source/WebKit2/UIProcess/PageClient.h:255
> +    virtual void startIndicatingView() = 0;
> +    virtual void stopIndicatingView() = 0;

Same bad naming.

> Source/WebKit2/UIProcess/ios/WKContentView.mm:64
> +@interface IndicateView : UIView

I think this class name should contain "inspector". It should have a prefix (WK or Web?), maybe with an underscore. Web/WKInspectorIndicationView?
Comment 3 Simon Fraser (smfr) 2014-03-24 23:10:02 PDT 
I'm OK with the patch, BTW, but I do object to the naming.
Comment 4 Joseph Pecoraro 2014-03-25 12:11:16 PDT 
Created attachment 227780 [details]
[PATCH] Renamed methods and classes
Comment 5 WebKit Commit Bot 2014-03-25 12:12:46 PDT 
Attachment 227780 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WKContentView.h:56:  Missing spaces around =  [whitespace/operators] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Simon Fraser (smfr) 2014-03-25 12:14:36 PDT 
Comment on attachment 227780 [details]
[PATCH] Renamed methods and classes

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

> Source/WebKit2/UIProcess/WebPageProxy.h:1325
> +    void startIndicating();
> +    void stopIndicating();

"indicating" is still too vague here. There may be many kinds of indication, of which Inspector overlays may be just one.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:472
> +    void indicate();
> +    void hideIndication();

Also too vague, and annoyingly different terminology to that on PageClient.
Comment 7 Joseph Pecoraro 2014-03-25 13:26:32 PDT 
Created attachment 227787 [details]
[PATCH] More renames
Comment 8 WebKit Commit Bot 2014-03-25 13:27:15 PDT 
Attachment 227787 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WKContentView.h:56:  Missing spaces around =  [whitespace/operators] [4]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Simon Fraser (smfr) 2014-03-25 13:41:08 PDT 
Comment on attachment 227787 [details]
[PATCH] More renames

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

> Source/WebKit/mac/WebView/WebView.mm:1896
> +- (void)setShowingInspectorIndication:(BOOL)enabled

enabled -> showing?
Comment 10 Tim Horton 2014-03-26 01:59:40 PDT 
This landed in http://trac.webkit.org/changeset/166257 as far as I can tell.
Comment 11 Joseph Pecoraro 2014-03-26 11:04:43 PDT 
Yes it did, thanks!