631934 - Remove childHitResult.shouldZoom switch in TabItem_zoomIn

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Michael Yoshitaka Erlewine [:mitcho]]

The idea with childHitResult.shouldZoom is that GroupItem_childHit is able to return a false value to block the zoomin, but this actually isn't used, as far as I can see. We should simply remove this bit of logic from TabItem_zoomIn and GroupItem_childHit, instead using GroupItem_childHit to simply be called to return the callback, if necessary.

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 2

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 510191 [details] [diff] [review]
v1

This is what I had in mind but, now that I look at it, I realize I think we could get rid of childHit completely... instead, in TabItem_zoomIn's onZoomDone, add a line that says:

> if (self.parent)
>   self.parent.collapse();

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: v1

Removed the childHit method.

Comment 4

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 510207 [details] [diff] [review]
v1

>+      if (!groupItem._isStacked || groupItem.expanded)
>+        shouldGroupCollapse = true;

This actually needs to simply be |if (groupItem.expanded)|. This is bug 631936, but we could just fix it here.

>+      else
>+        GroupItems.setActiveGroupItem(groupItem);

Note: setActiveGroupItem will be removed from this section in bug 597980. I'll leave it up to Ian to decide how separate we want these patches to be, or, because they're all touching the same section of code, if we want to roll them all into one patch.

f+ with the if condition changed.

Comment 5

[Ian Gilman [:iangilman]]

Comment on attachment 510207 [details] [diff] [review]
v1

(In reply to comment #4)
> >+      if (!groupItem._isStacked || groupItem.expanded)
> >+        shouldGroupCollapse = true;
> 
> This actually needs to simply be |if (groupItem.expanded)|. This is bug 631936,
> but we could just fix it here.

Yeah, we should just make the change here and retire that bug. 

> >+      else
> >+        GroupItems.setActiveGroupItem(groupItem);
> 
> Note: setActiveGroupItem will be removed from this section in bug 597980. I'll
> leave it up to Ian to decide how separate we want these patches to be, or,
> because they're all touching the same section of code, if we want to roll them
> all into one patch.

Let's land bug 597980 separately; that one actually fixes a bug, whereas this is just code clean-up.

Speaking of which, while code clean-up is certainly valuable, I think we're past the point where we should be doing it before the Fx4 release. It distracts us from fixing user-visible bugs (which should be our priority at this point), and it has the potential to cause new problems. That said, I'm okay with this patch (once it's been fixed up), as long as the drivers are, but please no more purely code clean-up patches before Fx4. Please do keep filing code clean-up bugs, but let's actually fix them after Fx4.

>+    let shouldGroupCollapse = false;
>+    let groupItem;
>+
>+    if (this.parent) {
>+      groupItem = this.parent;
>+
>+      if (!groupItem._isStacked || groupItem.expanded)
>+        shouldGroupCollapse = true;
>+      else
>+        GroupItems.setActiveGroupItem(groupItem);
>+    }

Once bug 597980 lands, we can get rid of all of this, in favor of modifying this:

>+      if (shouldGroupCollapse)
>+        groupItem.collapse();

... to this: 

  if (self.parent && self.parent.expanded)
    self.parent.collapse();
    
Don't forget to add the self = this declaration back in, of course.

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: v1

Only land this after bug 597980 has landed.

Comment 7

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 510508 [details] [diff] [review]
v1

Not sure why this is called v1, but f+.

+ Gavin for quick review while Ian is out.

Comment 8

[Ian Gilman [:iangilman]]

Comment on attachment 510508 [details] [diff] [review]
v1

Looks good

Comment 9

[Dietrich Ayala (:dietrich)]

What's the change in behavior with this patch? Any? Or is it just code cleanup?

Did it pass try?

Does the change need a new test, or does an existing test flex this codepath?

Comment 10

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #9)
> What's the change in behavior with this patch? Any? Or is it just code cleanup?

None. Just cleanup.

> Did it pass try?

Looks like it hasn't been sent. I'll send it now.

> Does the change need a new test, or does an existing test flex this codepath?

It's hit by multiple tests.

Comment 11

[Michael Yoshitaka Erlewine [:mitcho]]

Pushed to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=f2fa22612d4d

Comment 12

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #11)
> Pushed to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=f2fa22612d4d

Passed try, modulo two known intermittent oranges.

Comment 13

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 14

[Raymond Lee [:raymondlee]]

Please check in the patch for bug 597980 before this one.

Comment 15

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/cc7de25eaf70