Patch coming.
Created attachment 27189 [details] patch
Comment on attachment 27189 [details] patch > +JSValuePtr JSHTMLFormElement::submit(ExecState* exec, const ArgList&) > +{ > + Frame* activeFrame = asJSDOMWindow(exec->dynamicGlobalObject())->impl()->frame(); > + if (!activeFrame) > + return jsUndefined(); > + static_cast<HTMLFormElement*>(impl())->submit(0, false, false); > + return jsUndefined(); > +} I don't understand this change. Maybe the answer lies in the term "scaffolding'. We normally try not to make custom bindings unless we need to. Why does the activeFrame need to be checked for 0? > +bool ScriptController::pageIsProcessingUserGesture() const I don't think the word "page" in this function name is helpful. Also, wouldn't we also want to return true if another page in this page group was processing a user gesture? I'm not clear on how whether a frame is in one window or another affects this question. It seems that a frame opened with window.open that opens in a separate window should be no different from a frame in the same window for this check. > + for (Frame* frame = page->mainFrame(); frame; frame = frame->tree()->traverseNext()) > + if (frame->script()->processingUserGesture()) > + return true; Please add braces. > case FrameLoadTypeRedirectWithLockedHistory: > - // no-op: leave as protocol default > - // FIXME: I wonder if we ever hit this case > break; Is no comment at all really the best thing to leave here? > void loadPostRequest(const ResourceRequest& inRequest, const String& referrer, // Called by loadFrameRequestWithFormAndValues(), calls loadWithNavigationAction > - const String& frameName, Event* event, PassRefPtr<FormState> prpFormState); > + const String& frameName, FrameLoadType loadType, Event* event, PassRefPtr<FormState> prpFormState); Lets leave out the names "inRequest", "loadType", "event", and "prpFormState". I didn't review the tests carefully. r=me
(In reply to comment #2) > (From update of attachment 27189 [details] [review]) > > +JSValuePtr JSHTMLFormElement::submit(ExecState* exec, const ArgList&) > > +{ > > + Frame* activeFrame = asJSDOMWindow(exec->dynamicGlobalObject())->impl()->frame(); > > + if (!activeFrame) > > + return jsUndefined(); > > + static_cast<HTMLFormElement*>(impl())->submit(0, false, false); > > + return jsUndefined(); > > +} > > I don't understand this change. Maybe the answer lies in the term > "scaffolding'. We normally try not to make custom bindings unless we need to. It's a custom binding because the code generator only knows how to generate a call to "submit()" -- it doesn't know how to supply arguments that were not supplied by JavaScript. Eventually, JSHTMLFormElement will use a userGesture check to decide what arguments to pass to submit(). > Why does the activeFrame need to be checked for 0? That's the idiom elsewhere. Maybe it's not necessary. I'm not sure. > > +bool ScriptController::pageIsProcessingUserGesture() const > > I don't think the word "page" in this function name is helpful. Also, wouldn't > we also want to return true if another page in this page group was processing a > user gesture? I'm not clear on how whether a frame is in one window or another > affects this question. It seems that a frame opened with window.open that opens > in a separate window should be no different from a frame in the same window for > this check. I'll add code to check all the pages in the group. > > + for (Frame* frame = page->mainFrame(); frame; frame = frame->tree()->traverseNext()) > > + if (frame->script()->processingUserGesture()) > > + return true; > > Please add braces. OK. > > > case FrameLoadTypeRedirectWithLockedHistory: > > - // no-op: leave as protocol default > > - // FIXME: I wonder if we ever hit this case > > break; > > Is no comment at all really the best thing to leave here? I didn't find the FIXME helpful because it didn't describe a course of action, or a why. I didn't find "no-op: leave as protocol default" helpful because I know that break means "no-op", so that part's irrelevant, but I don't know what's being left, or what "protocol default" means, so that part's confusing. > > void loadPostRequest(const ResourceRequest& inRequest, const String& referrer, // Called by loadFrameRequestWithFormAndValues(), calls loadWithNavigationAction > > - const String& frameName, Event* event, PassRefPtr<FormState> prpFormState); > > + const String& frameName, FrameLoadType loadType, Event* event, PassRefPtr<FormState> prpFormState); > > Lets leave out the names "inRequest", "loadType", "event", and "prpFormState". OK.
Committed revision 40424.