[webkit-reviews] review denied: [Bug 76014] Web Intents chromium API modifications to track IntentRequest invocation method : [Attachment 122445] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Greg Billock
<gbillock at google.com>'s request for review:
Bug 76014: Web Intents chromium API modifications to track IntentRequest
invocation method
https://bugs.webkit.org/show_bug.cgi?id=76014

Attachment 122445: Patch
https://bugs.webkit.org/attachment.cgi?id=122445&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=122445&action=review


> Source/WebKit/chromium/public/WebFrameClient.h:388
> +    virtual void dispatchIntent(WebFrame*, WebIntentRequest*) { }

WebIntentRequest should be passed using |const Foo&|.  the chromium code should

retain a WebIntentRequest by using the copy-constructor.  the only oddity about

this is that it strips the const, but that's the pattern we've been following.
treat it like a RefPtr.

> Source/WebKit/chromium/public/WebIntent.h:57
> +    // FIXME(gbillock): delete this.

nit: WebKit style does not put names on FIXME comments like this.

> Source/WebKit/chromium/public/WebIntent.h:62
> +    WebIntent(const WebIntent&);

why don't you want to allow the embedder to copy a WebIntent?  i think
you will have trouble using WebIntentRequest::intent() if you don't
make the copy-constructor available.

There's a pattern for writing classes like this.  For an example that
is simpler than WebNode, see WebUserMediaRequest.  You should have
the following methods generally speaking on classes that just wrap
a ref-counted WebCore type:

  default constructor
  copy constructor
  destructor
  operator=
  reset
  isNull
  assign

you can optionally add equals and operator== if needed.

treat this as boilerplate.  maybe we should have macros to make this
easier to replicate, but we've so far favored the readability of not
using macros.

same deal for WebIntentRequest.h