In HTMLInputElement.cpp, we create a new thread when opening a file picker or a color picker (ASyncClickerHandler). This was necessary but it's not anymore, as all pickers on all platforms are supposed to be asynchronous already now.

I removed the creation of the thread as well as the AsyncClickHandler class which is not needed anymore, and move its methods back into HTMLInputElement. I also took the opportunity of this patch to factorize some of the code of Init{Color,File}Picker methods which check popup permissions. Mounir, what do you think about this?

Comment on attachment 781822 [details] [diff] [review] Remove useless thread patch Review of attachment 781822 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for working on this :) The patch looks good but I can't r+ it because there are a few changes that I would like to see and review. Also, I will have to spend a bit more time thinking about the design (even though, I think it is okay) but I wanted to give you some early feedback. ::: content/html/content/src/HTMLInputElement.cpp @@ +482,5 @@ > + // Get parent nsPIDOMWindow object. > + nsCOMPtr<nsIDocument> doc = OwnerDoc(); > + > + nsCOMPtr<nsPIDOMWindow> win = doc->GetWindow(); > + NS_ASSERTION(win, "window shouldn't not be null"); NS_ASSERTION() -> MOZ_ASSERT() and add: if (!win) { return false; } Otherwise, feel free to use MOZ_CRASH(). @@ +487,5 @@ > + > + PopupControlState popupControlState = win->GetPopupControlState(); > + > + // Check if page is allowed to open the popup > + if (popupControlState > openControlled) { I guess you can do: if (win->GetPopupControlState() > openControlled) { Even better: if (win->GetPopupControlState() <= openControlled) { return false; } // the rest, no longer indented. @@ +499,5 @@ > + uint32_t permission; > + pm->TestPermission(doc->NodePrincipal(), &permission); > + if (permission == nsIPopupWindowManager::DENY_POPUP) { > + return true; > + } return permission == nsIPopupWindowManager::DENY_POPUP. @@ +513,5 @@ > > nsCOMPtr<nsPIDOMWindow> win = doc->GetWindow(); > if (!win) { > return NS_ERROR_FAILURE; > } We no longer need all of that code for the popup blocker code. Move it after the popup check. @@ +552,3 @@ > if (!win) { > return NS_ERROR_FAILURE; > } ditto @@ +659,5 @@ > > nsCOMPtr<nsISupports> container = aDoc->GetContainer(); > nsCOMPtr<nsILoadContext> loadContext = do_QueryInterface(container); > > + nsCOMPtr<nsIContentPrefCallback2> prefCallback = nit: plz revert this change :) @@ +2897,5 @@ > + if (GetType() == NS_FORM_INPUT_FILE) { > + InitFilePicker(); > + } else if (GetType() == NS_FORM_INPUT_COLOR) { > + InitColorPicker(); > + } I guess you can also make it a switch on mType. (I would recommend using mType instead of GetType() FWIW.) @@ +2912,1 @@ > return NS_OK; You should think about whether or net we should have a return value for MaybeInitPickers. InitColorPicker() and InitFilePicker() both return a nsresult. Is that really needed? If the result they return actually make sense, we might want to get MaybeInitPickers() returning a nsresult too, in which case, we should do: return MaybeInitPickers(aVisitor); here. @@ +3316,2 @@ > > return rv; ditto ::: content/html/content/src/HTMLInputElement.h @@ +1093,5 @@ > + /** > + * Use this function before trying to open a picker. > + * It checks if the page is allowed to open a new pop-up. > + * If it returns true, you should not create the picker. > + * @return true if popup should be blocked, false otherwise nit: leave an empty line before @return line. @@ +1140,5 @@ > * a change event. This is to ensure correct future change event firing. > * NB: This is ONLY applicable where the element is a text control. ie, > * where type= "text", "email", "search", "tel", "url" or "password". > */ > + nsString mFocusedValue; Please, don't change the trailing whitespaces. It makes the patch bigger and will pollute the blame for not much reasons. @@ +1229,5 @@ > } else { > return false; > } > } > + ditto @@ +1241,5 @@ > // hard-coded set). > // false means it may come from an "untrusted" source (e.g. OS mime types > // mapping, which can be different accross OS, user's personal configuration, ...) > // For now, only mask filters are considered to be "trusted". > + bool mIsTrusted; And here.

(In reply to Mounir Lamouri (:mounir) from comment #2) > Please, don't change the trailing whitespaces. It makes the patch bigger and > will pollute the blame for not much reasons. Yep, I know, but I used Eclipse and it's configured this way by default, and I found this to be pretty useful, to remove my own trailing spaces I might added when coding. It's sad not everyone working on the code base is taking care of this :( But I think I will have to turn off this feature.

FWIW, I have a configuration in vim that highlights the trailing whitespaces and I manually remove them. That way, I usually don't remove trailing whitespaces by mistake but I also usually don't add some.

OK, sounds like a good idea indeed :) I think I should configure something somewhat similar here. Thanks for the idea.

About high level design, I remember we talked about this and the necessity of removing this now useful thread. But do you think it's worth it? I know all implementations of pickers are supposed to be async, making this thread useless, but can we assume they will always be implemented correctly to guarantee this? (i.e. can we trust the implementations?). If not, it might safer to keep this thread, to avoid the UI to be freezed because of a bad implementation. Also, I haven't checked deeply, but I guess the overhead isn't that big: I guess a new thread isn't created each time but there is a pool of thread instead, where some thread are already ready to do some work. I have no strong opinion about this, and I don't know what is the best solution here: I just wanted to highlight some things I'm wondering about. I you're sure it's worth to change this, I'm still OK to write the patch to do so ;)

I believe it is worth it. It makes the code simpler to read and we might indeed have bad implementations but all the callers of the async method of nsIFilePicker will assume the method is async. Not having it async would be terrible for more than just HTMLInputElement. This said, if your time is limited and would like to hack other things, this is not very high priority and feel free to leave this bug for later.

It's not a big change and I didn't spent lot of time on this, so I'm OK to work on this. Do you need more time to review the patch, design at a higher level? Or can I resubmit an updated patch with your comments applied?

(In reply to Arnaud from comment #8) > Do you need more time to review the patch, design at a higher level? Or can > I resubmit an updated patch with your comments applied? You should apply another patch with my comments. I will do a final review at that moment. I think it should be fine.

(In reply to Mounir Lamouri (:mounir) from comment #2) > > > > nsCOMPtr<nsPIDOMWindow> win = doc->GetWindow(); > > if (!win) { > > return NS_ERROR_FAILURE; > > } > > We no longer need all of that code for the popup blocker code. Move it after > the popup check. Which code? We need "doc" and "win" before calling "IsPopupBlocked" because we need them as arguments for "FirePopupBlockedEvent" we will execute if true. And we need "win" after (for Init). Maybe you're talking about this "if (!win)" check. In that case, indeed we can get rid of it as we do the check in the "IsPopupBlocked" and we will not go further if "IsPopupBlocked" is false. But in that case we will return "NS_OK" instead of "NS_ERROR_FAILURE", because we are not able to know why "IsPopupBlocked" returns false. This is if I use MOZ_ASSERT in "IsPopupBlocked": if I use MOZ_CRASH, the test isn't needed anymore indeed. > You should think about whether or net we should have a return value for > MaybeInitPickers. InitColorPicker() and InitFilePicker() both return a > nsresult. Is that really needed? If the result they return actually make > sense, we might want to get MaybeInitPickers() returning a nsresult too I kept the existing interface but indeed, I think it makes sense now: this was not possible before as the operation was async. Now we can know of the init was fine or not. > @@ +3316,2 @@ > > > > return rv; About this one, in PostHandleEvent, we will ignore the current value of rv (which might be set to NS_FAILURE) if we do this. Maybe it would be better to call "return MaybeInitPickers" only if "NS_SUCCEEDED(rv)", and return rv otherwise. But this will change the current behavior, where we call "MaybeInitPickers" no matter what the value of rv is. Or maybe it's not important, if this is mostly sanity checks.

Here is an updated version of the patch, with your comments applied, except the code move you requested, for which I asked clarification in my previous comment 10.

And here is the interdiff.

Comment on attachment 784474 [details] [diff] [review] Remove useless thread (2nd) Review of attachment 784474 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the comments applied. Thanks :) ::: content/html/content/src/HTMLInputElement.cpp @@ +478,5 @@ > > +bool > +HTMLInputElement::IsPopupBlocked() const > +{ > + // Get parent nsPIDOMWindow object. nit: I think you can get ride of that comment ;) @@ +481,5 @@ > +{ > + // Get parent nsPIDOMWindow object. > + nsCOMPtr<nsIDocument> doc = OwnerDoc(); > + > + nsCOMPtr<nsPIDOMWindow> win = doc->GetWindow(); I guess you can also not have a doc variable and simply do: nsCOMPtr<nsPIDOMWindow> win = OwnerDoc()->GetWindow(); @@ +484,5 @@ > + > + nsCOMPtr<nsPIDOMWindow> win = doc->GetWindow(); > + MOZ_ASSERT(win, "window should not be null"); > + if (!win) { > + return false; I would prefer to return true in that case. @@ +491,5 @@ > + // Check if page is allowed to open the popup > + if (win->GetPopupControlState() <= openControlled) { > + return false; > + } > + nit: trailing whitespace @@ +493,5 @@ > + return false; > + } > + > + nsCOMPtr<nsIPopupWindowManager> pm = do_GetService(NS_POPUPWINDOWMANAGER_CONTRACTID); > + nit: you could get ride of that empty line. @@ +499,5 @@ > + return true; > + } > + > + uint32_t permission; > + pm->TestPermission(doc->NodePrincipal(), &permission); Here you could do OwnerDoc()->NodePrincipal() if you remove the |doc| variable. @@ +2895,4 @@ > !aVisitor.mEvent->mFlags.mDefaultPrevented) { > + if (mType == NS_FORM_INPUT_FILE) { > + return InitFilePicker(); > + } else if (mType == NS_FORM_INPUT_COLOR) { Having a |else| after a return is useless. Just write something like: if (mType == NS_FORM_INPUT_FILE) { return InitFilePicker(); } if (mType == NS_FORM_INPUT_COLOR) { return InitColorPicker(); } ::: content/html/content/src/HTMLInputElement.h @@ +1093,5 @@ > + /** > + * Use this function before trying to open a picker. > + * It checks if the page is allowed to open a new pop-up. > + * If it returns true, you should not create the picker. > + * nit: trailing whitespace

Updated version of the patch, with Mounir's comments applied.

For the record, the interdiff.

Comment on attachment 784935 [details] [diff] [review] Remove useless thread (3rd) Just set checkin-needed please.

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16) > Just set checkin-needed please. Sorry, I wasn't sure checkin-needed was enough. Also I thought additionally marking the patch/attachment to be check-in would be better, to avoid ambiguity. Just out of curiosity, what's the purpose of attachment checkin flag so?