Closed Bug 1310942 Opened 8 years ago Closed 8 years ago

Merge browser-autocomplete-result-popup into browser-search-autocomplete-result-popup

Categories

(Firefox :: Search, defect, P3)

Product:
Firefox
Component:
Search
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: mak, Assigned: jordan, Mentored)

References

Details

(Whiteboard: [fxsearch][good first bug][lang=js])

Attachments

(1 file)

Bug 1310942 - Merge browser-autocomplete-result-popup into browser-search-autocomplete-result-popup;
(deleted), text/x-review-board-request
mak
: review+
Details
After bug 1296638, browser-search-autocomplete-result-popup is the only consumer of browser-autocomplete-result-popup, so we'd better merge them. We could evaluate moving also the search bar to the RLB implementation, just because it will be the version getting the major attention, but the benefit may not be tangible until we want to style its contents in some fancy way.
Mentor: mak77
Priority: -- → P3
Whiteboard: [fxsearch][good first bug][lang=js]
hi Marco, I'd like to work on this bug. Please give me directions. If, I'm right, I'd have to move the code of "#browser-autocomplete-result-popup" of browser/base/content/urlbarBindings.xml file to "#browser-search-autocomplete-result-popup" of browser/components/search/content/search.xml file. Please, assign me this bug. Thank You!
yes, the 2 implementations should be merged into search.xml.
Assignee: nobody → souravgarg833
Status: NEW → ASSIGNED
Marco, I've attached a patch file. Am I supposed to perform any automated test? If so, then how to find appropriate test? Also, is there any need to attach some test, & how to create them? Please assist me. Thanks.
I don't think in this case you can attach any new meaningful tests. If you have commit access first level you should be able run a Try run on our Try Server from the automation menu on moz-review, you can use these options try: -b do -p win32,win64,linux64,linux,macosx64 -u firefox-ui-functional,marionette,marionette-e10s,mochitests,xpcshell -t none Locally you could run tests using mach test browser/components/search/test it may take some time
Comment on attachment 8802757 [details] Bug 1310942 - Merge browser-autocomplete-result-popup into browser-search-autocomplete-result-popup; https://reviewboard.mozilla.org/r/87056/#review86558 ::: browser/components/search/content/search.xml:882 (Diff revision 1) > > </handlers> > </binding> > > - <binding id="browser-search-autocomplete-result-popup" extends="chrome://browser/content/urlbarBindings.xml#browser-autocomplete-result-popup"> > + <binding id="browser-search-autocomplete-result-popup" extends="chrome://global/content/bindings/autocomplete.xml#autocomplete-result-popup"> > + <implementation> there is already an implementation tag in this binding, you should just move the methods from the other implementation to the one that already exists. ::: browser/components/search/content/search.xml:886 (Diff revision 1) > - <binding id="browser-search-autocomplete-result-popup" extends="chrome://browser/content/urlbarBindings.xml#browser-autocomplete-result-popup"> > + <binding id="browser-search-autocomplete-result-popup" extends="chrome://global/content/bindings/autocomplete.xml#autocomplete-result-popup"> > + <implementation> > + <field name="AppConstants" readonly="true"> > + (Components.utils.import("resource://gre/modules/AppConstants.jsm", {})).AppConstants; > + </field> > + you created some trailing spaces in the move, please remove all the trailing spaces. many text editors have an option to remove trailing spaces on the lines you modify
Attachment #8802757 - Flags: review?(mak77) → review-
hi, I've used xcode "Automatically trim trailing whitespace" feature to remove the whitespace & then --amend this commit to previous commit. Please review the updated patch. Thanks!
Comment on attachment 8802757 [details] Bug 1310942 - Merge browser-autocomplete-result-popup into browser-search-autocomplete-result-popup; https://reviewboard.mozilla.org/r/87056/#review87044 ::: browser/components/search/content/search.xml:905 (Diff revision 2) > </xul:tree> > <xul:vbox anonid="search-one-off-buttons" class="search-one-offs"/> > </content> > <implementation> > + <field name="AppConstants" readonly="true"> > + (Components.utils.import("resource://gre/modules/AppConstants.jsm", {})).AppConstants; yout editor is using 4 spaces indentation instead of 2 spaces one. ::: browser/components/search/content/search.xml:912 (Diff revision 2) > + > + <method name="openAutocompletePopup"> > + <parameter name="aInput"/> > + <parameter name="aElement"/> > + <body> > + <![CDATA[ please put CDATA inline with body as <body><![CDATA[ and reindent its contents properly ::: browser/components/search/content/search.xml:927 (Diff revision 2) > + <method name="onPopupClick"> > + <parameter name="aEvent"/> > + <body><![CDATA[ > + // Ignore all right-clicks > + if (aEvent.button == 2) > + return; Sorry, but looks like you have lost all the indentation information in moving the code? We should retain the original indentation, please. ::: browser/components/search/content/search.xml:975 (Diff revision 2) > + else > + searchBar.value = search; > + } > + ]]></body> > + </method> > + everything looks good, but you still introduced some trailing spaces here
Attachment #8802757 - Flags: review?(mak77) → review-
I'm sorry for all the trouble. If it still have trailing spaces, please suggest me a way to check for them. ::: browser/components/search/content/search.xml:912 actually the original file had, body & CDATA tags in different lines, but anyway I've fixed it. Thanks!
(In reply to Sourav Garg [:jordan] from comment #11) > I'm sorry for all the trouble. If it still have trailing spaces, please > suggest me a way to check for them. Sublime Text has a Trailing Spaces package that can remove them from touched lines, it can also hilight them, so they are easy to see. I think other editors have similar addons or features. Unfortunately Atom has the issue still open https://github.com/atom/whitespace/issues/8
Comment on attachment 8802757 [details] Bug 1310942 - Merge browser-autocomplete-result-popup into browser-search-autocomplete-result-popup; https://reviewboard.mozilla.org/r/87056/#review87566 ::: browser/components/search/content/search.xml:937 (Diff revision 3) > + var popupForSearchBar = searchBar && searchBar.textbox == this.mInput; > + if (popupForSearchBar) { > + searchBar.telemetrySearchDetails = { > + index: controller.selection.currentIndex, > + kind: "mouse" > + }; Sorry, you properly fixed <body> and the 2 spaces indentation, but you still lost all indentation in the moved code. Please copy/paste it again and try to preserve its indentation. ::: browser/components/search/content/search.xml:976 (Diff revision 3) > + else > + searchBar.value = search; > + } > + ]]></body> > + </method> > + yes, you still have trailing spaces here.
Attachment #8802757 - Flags: review?(mak77) → review-
Any news here? Can I help you anyway?
Flags: needinfo?(souravgarg833)
Sorry Sir, I got engaged with my end-semester exams. I'll get back to work after 21st November.
Marco, please have a look at the refixed attachment.
Comment on attachment 8802757 [details] Bug 1310942 - Merge browser-autocomplete-result-popup into browser-search-autocomplete-result-popup; https://reviewboard.mozilla.org/r/87056/#review97134 It looks great now, Thank you!
Attachment #8802757 - Flags: review?(mak77) → review+
Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/19cd461b6a06 Merge browser-autocomplete-result-popup into browser-search-autocomplete-result-popup; r=mak
https://hg.mozilla.org/mozilla-central/rev/19cd461b6a06
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Flags: needinfo?(souravgarg833)
Blocks: 1427350
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: