Closed
Bug 879631
Opened 11 years ago
Closed 11 years ago
Change - Remove form navigation code
Categories
(Firefox for Metro Graveyard :: App Bar, defect, P2)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: rsilveira, Assigned: jwilde)
References
Details
(Whiteboard: [shovel-ready] feature=change c=Find_in_page_app_bar u=metro_firefox_user p=1 status=verified)
Attachments
(1 file, 2 obsolete files)
10.05 KB,
patch
|
mbrubeck
:
review+
|
Details | Diff | Splinter Review |
Quoting mbrubeck from comment 4 on bug 879115: "At some point we should simplify the content-navigator code. It used to show not only the findbar but also the "form navigator" (long since removed). We don't need all that functionality anymore."
Updated•11 years ago
|
Blocks: metrov1defect&change
Summary: Remove form navigation code → Change - Remove form navigation code
Whiteboard: [shovel-ready] → [shovel-ready] feature=change c=Find_in_page_app_bar u=metro_firefox_user p=0
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → hello
Assignee | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•11 years ago
|
||
Drops the formhelper components from the application.
Assignee | ||
Updated•11 years ago
|
Attachment #761155 -
Attachment is patch: true
Attachment #761155 -
Attachment mime type: text/x-patch → text/plain
Assignee | ||
Updated•11 years ago
|
Attachment #761155 -
Flags: review?(mbrubeck)
Comment 2•11 years ago
|
||
Comment on attachment 761155 [details] [diff] [review] patch v1 Review of attachment 761155 [details] [diff] [review]: ----------------------------------------------------------------- This is too far-reaching. Sorry, I should have been more specific in my comment. We do use the parts of FormHelper that handle autocompletion, select menus, and scrolling to show selected elements. The main thing that is unused and should be removed is base/content/bindings/bindings.xml#content-navigator - we used to use this binding to make the "navigator" bar change behavior depending on whether it was in "find in page" mode or "form helper" mode. Now we have only find-in-page, so we can ditch this binding and just put the toolbar content directly into browser.xul. ::: browser/metro/base/content/browser.xul @@ -114,5 @@ > > - <!-- forms navigation --> > - <command id="cmd_formPrevious" oncommand="FormHelperUI.goToPrevious();"/> > - <command id="cmd_formNext" oncommand="FormHelperUI.goToNext();"/> > - <command id="cmd_formClose" oncommand="FormHelperUI.hide();"/> Yeah, this stuff is dead code and should be removed. ::: browser/metro/base/content/helperui/SelectHelperUI.js @@ -30,5 @@ > show: function selectHelperShow(aList, aTitle, aRect) { > - if (AnimatedZoom.isZooming()) { > - FormHelperUI._waitForZoom(this.show.bind(this, aList, aTitle, aRect)); > - return; > - } Yes, this can be removed too.
Attachment #761155 -
Flags: review?(mbrubeck) → review-
Updated•11 years ago
|
Blocks: metrov1it9
Updated•11 years ago
|
No longer blocks: metrov1defect&change
Comment 3•11 years ago
|
||
Hi Jonathan, I can meet with you on Thursday when I return from vacation. In the meantime, I'll need a point estimation for this Change Story you have selected for Iteration #9. I've copied Brian Bondy to give you a brief on how our point estimations works.
Flags: needinfo?(netzen)
Flags: needinfo?(hello)
QA Contact: jbecerra
Comment 4•11 years ago
|
||
So points are basic estimations for level of effort. If you can do something in a day I usually just put 1 point. Possible point values are basically Fibonacci numbers, i.e. 1,2,3,5,8,13. If you are going beyond that, we probably want to split up the bug instead into multiple parts. If you go look in the Whiteboard of any Metro bug, you'll see this included in the text: p=SomeNumber. SomeNumber is the number of points assigned. If no value is assigned yet you'll see p=0. Sometimes the header is missing and you can just CC Marco or a QA member to help you fill it out. If a bug is in the current iteration and SomeNumber is set to 0, feel free to put your point estimation in. If you want to provide a point estimation for any bug not in the current iteration, just put a new comment with p=WhateverYouThink. When the iteration starts the comments with p=WhateverYouThink will usually be moved up into the Whiteboard header. Note there is a distinction between: p=1 and p1. The former is a point value, the later is the importance level of the bug. We use both so just note if there's an '=' then we're talking about points and if there is no '=' then we're talking about importance. The importance levels are set in the Importance Bugzilla header instead of the Whiteboard Bugzilla header. P1 usually means fix next iteration, P2 means fix before release but maybe not next iteration. P3 means we probably want to fix before release but we'll re-evaluate. P4 or higher means it's a v2 item.
Flags: needinfo?(netzen)
Assignee | ||
Comment 5•11 years ago
|
||
Ah, okay! All of that seems to make sense. Marco, would be glad to meet with you on Thursday. Brian, thanks for the overview! This really should be a p=1 bug, but I'm running a bit slow since I'm still getting a few dev environment things spun up.
Flags: needinfo?(hello)
Comment 6•11 years ago
|
||
Ya p=1 is fine, points are not really about time at all, just estimated level of effort that one would spend fixing it.
Whiteboard: [shovel-ready] feature=change c=Find_in_page_app_bar u=metro_firefox_user p=0 → [shovel-ready] feature=change c=Find_in_page_app_bar u=metro_firefox_user p=1
Assignee | ||
Comment 7•11 years ago
|
||
Replaces use of content-navigator xbl with appbar xbl, removes various bit of older code, and fixes all of the alignment issues.
Attachment #761155 -
Attachment is obsolete: true
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #761591 -
Attachment is obsolete: true
Attachment #761593 -
Flags: review?(mbrubeck)
Assignee | ||
Comment 9•11 years ago
|
||
New patch was posted because the old one added spaces on blank lines.
Comment 10•11 years ago
|
||
Comment on attachment 761593 [details] [diff] [review] patch v2.1 Review of attachment 761593 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #761593 -
Flags: review?(mbrubeck) → review+
Assignee | ||
Comment 11•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/65cc101ff835
Comment 12•11 years ago
|
||
Hey Brian. Thanks very much for providing the detail explanation. Your points in Comment #4 and #6 were perfect. Hi Jonathan, thanks very much for providing the point estimate. I'll send you a meeting request for Thursday to meet.
Updated•11 years ago
|
Priority: -- → P2
Comment 13•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/65cc101ff835
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 14•11 years ago
|
||
Tested for iteration #9 on 2013-07-10 using latest nightly. I'm not sure how to test this, so I tried making sure the functionality in the find bar and form filling was working as expected, which it is for the most part. I'm filling defect bugs for issues that I saw like bug 892224 but which may not be related to this.
Status: RESOLVED → VERIFIED
Whiteboard: [shovel-ready] feature=change c=Find_in_page_app_bar u=metro_firefox_user p=1 → [shovel-ready] feature=change c=Find_in_page_app_bar u=metro_firefox_user p=1 status=verified
Comment 15•10 years ago
|
||
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 Build ID: 20130826074752 Built from http://hg.mozilla.org/mozilla-central/rev/14b1e8c2957e WFM Tested on windows 8 using latest nightly for iteration-12. Followed steps provided in comment0 and got expected result.I tried making sure the functionality in the find bar and form filling was working as expected, which it is for the most part.
Updated•10 years ago
|
OS: Windows 8 Metro → Windows 8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•