Pass the invoked JavaScript method name as an argument to the fallback callback

Pass the nonexistent JavaScript method name as the first parameter in the argument list to the fallback handler in CppBoundClass.  Also, return true from
CppBoundClass::HasMethod() in all cases if a fallback handler is assigned (this is how the code we previously documented, but not how it actually behaves).

[Evan Martin, 2008-09-19 20:48:17]

More important than my comments below is that you fix the other code that relies
on this change.  I guess you'd need to run the layout tests with this change in
place and then fix the tests that were broken.  It might be easier to grep the
code for uses of the fallback callback and try to fix them first.

http://codereview.chromium.org/3175/diff/1/2
File webkit/glue/cpp_bound_class.cc (right):

http://codereview.chromium.org/3175/diff/1/2#newcode170
Line 170: size_t offset = 0;
This code is sorta mysterious if you don't know what it's trying to accomplish. 
Maybe this could be
bool inject_method_name = false;

http://codereview.chromium.org/3175/diff/1/2#newcode186
Line 186: cpp_args[i].Set(args[i - offset]);
And then here, just .Set(args[i + (inject_method_name ? 1 : 0])
with a similar adjustment of the size of the list.

http://codereview.chromium.org/3175/diff/1/2#newcode190
Line 190: if (offset == 1) {
That would allow this if statement to test the boolean instead of this magic
value.

[Marshall Greenblatt, 2008-09-20 00:34:09]

Please review the updated patch that makes the code changes less magical.

[Evan Martin, 2008-09-20 00:42:36]

LGTM

I'll try to commit it on Monday...

[Evan Martin, 2008-09-24 20:08:56]

Have you filed the CLA?
http://dev.chromium.org/developers/contributing-code

If so, could you add yourself to the AUTHORS file in this patch?

[Marshall Greenblatt, 2008-09-25 02:00:17]

On 2008/09/24 20:08:56, Evan Martin wrote:
> Have you filed the CLA?
> http://dev.chromium.org/developers/contributing-code
> 
> If so, could you add yourself to the AUTHORS file in this patch?

Filed the CLA and added myself to the AUTHORS file.  Please see the updated
patch.

[Evan Martin, 2008-09-25 02:09:14]

I just remembered some of this code might not work.  Feng, do you remember
changing this code?  (It was in May 2008.)

http://codereview.chromium.org/3175/diff/7/211
File webkit/glue/cpp_bound_class.cc (right):

http://codereview.chromium.org/3175/diff/7/211#newcode157
Line 157: return (methods_.find(ident) != methods_.end() ||
fallback_callback_.get());
Hmm, I remember this causing problems in some bit of code.  I wonder if I could
find it... ah, yes.  The commit message was (from Feng):
"The fix [to not test fallback_callback] in cpp_bound_class.cc is a bonus. I
talked to evanm that the check in incorrect. It does not work properly with
NPObject binding code."

[Marshall Greenblatt, 2008-10-04 22:45:24]

Please review the updated patch set.  Fixed conflict in the AUTHORS file.

[Pam (message me for reviews), 2008-10-06 16:52:43]

In addition to whatever Feng might suggest, please add cases to
cpp_bound_class_unittest.cc (a new InvokeNonexistentMethodsWithFallback test) to
cover this change.

- Pam

[Evan Martin, 2008-10-07 18:26:42]

http://codereview.chromium.org/3175/diff/601/801
File webkit/glue/cpp_bound_class.cc (right):

http://codereview.chromium.org/3175/diff/601/801#newcode157
Line 157: return (methods_.find(ident) != methods_.end() ||
fallback_callback_.get());
I spoke with Feng.  This is the only line we have a problem with.  I'm sure we
used to have this and had to remove it for some reason, but neither Feng nor I
can remember why.  Do you really need this, or were you just trying to make it
match the the HasMethod() docs?

[Marshall Greenblatt, 2008-10-07 19:36:04]

With the code as it currently stands the fallback callback will never be called
even if one is configured.  This is because HasMethod() must return true before
Invoke() will be executed.

The proposed change to CppBoundClass::HasMethod() is therefore required so that
the fallback callback can be called.  So, we either need the proposed change to
CppBoundClass::HasMethod() or we should remove fallback callback support
completely.

[Evan Martin, 2008-10-07 19:50:57]

I found the change where this was removed.  The diff includes the following:

-  // If we have a fallback method, we have to return true even if there
-  // is no such method, otherwise invoke is not called.
-  if (fallback_callback_.get())
-    return true;

Unfortunately, the only thing noted in the commit log is
"The fix in cpp_bound_class.cc is a bonus. I talked to evanm that the check in
incorrect. It does not work properly with NPObject binding code."

BTW, you might be interested to look at the similar code in WebKit, which also
passes the function name:
http://src.chromium.org/viewvc/chrome/trunk/deps/third_party/WebKit/WebCore/b...
(search for "fallback").


I think if this passes the tests, it LGTM.  It looks like fallbacks are used by:
1) cpp_bound_class_unittest
2) the layout test controller (which means a layout test might change)

Again, really sorry about the delays -- they won't last.  Normally we should be
a day or two for turnaround.

[Marshall Greenblatt, 2008-10-07 20:22:24]

If I understand the WebKit code properly it looks like they pass the invoked
method name as a separate parameter.  We talked about that option but I believe
we disqualified it due to the scope of changes required.

Back before the webkit merge I ran the layout tests with this patch applied and
none of them failed for reasons associated with this change.   I'll implement
the additional test case that Pam requested and run the layout tests again in a
week or so once the merge is more settled.