Closed Bug 1064439 Opened 6 years ago Closed 5 years ago

Fix up nsDocShellTreeOwner and nsWebBrowser a little

Categories

(Core Graveyard :: Embedding: APIs, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla37

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(21 files)

23.98 KB, patch
jst
: review+
Details | Diff | Splinter Review
6.09 KB, patch
jst
: review+
Details | Diff | Splinter Review
14.67 KB, patch
jst
: review+
Details | Diff | Splinter Review
4.75 KB, patch
jst
: review+
Details | Diff | Splinter Review
10.73 KB, patch
jst
: review+
Details | Diff | Splinter Review
1.68 KB, patch
jst
: review+
Details | Diff | Splinter Review
2.10 KB, patch
jst
: review+
Details | Diff | Splinter Review
3.13 KB, patch
jst
: review+
Details | Diff | Splinter Review
4.52 KB, patch
jst
: review+
Details | Diff | Splinter Review
1.12 KB, patch
jst
: review+
Details | Diff | Splinter Review
965 bytes, patch
jst
: review+
Details | Diff | Splinter Review
24.32 KB, patch
jst
: review+
Details | Diff | Splinter Review
11.80 KB, patch
jst
: review+
Details | Diff | Splinter Review
1.10 KB, patch
jst
: review+
Details | Diff | Splinter Review
2.60 KB, patch
jst
: review+
Details | Diff | Splinter Review
4.27 KB, patch
jst
: review+
Details | Diff | Splinter Review
1.19 KB, patch
jst
: review+
Details | Diff | Splinter Review
3.75 KB, patch
jst
: review+
Details | Diff | Splinter Review
2.57 KB, patch
jst
: review+
Details | Diff | Splinter Review
3.51 KB, patch
jst
: review+
Details | Diff | Splinter Review
5.97 KB, patch
jst
: review+
Details | Diff | Splinter Review
No description provided.
Summary: Fix up the style a little in nsDocShellTreeOwner → Fix up nsDocShellTreeOwner a little
The patches 1a-1e are all just silly formatting things, so I'll land them merged into a single patch.

try run: https://tbpl.mozilla.org/?tree=Try&rev=9156076fd279
Attachment #8486647 - Flags: review?(jst)
This is odd.  The QI of DefaultTooltipTextProvider doesn't do anything special, so I'm not sure why this code is like this.
Attachment #8486663 - Flags: review?(jst)
Less white space changes in this series.

try run: https://tbpl.mozilla.org/?tree=Try&rev=9156076fd279
Attachment #8486671 - Flags: review?(jst)
I'll land the whitespace changes folded together.
Attachment #8486673 - Flags: review?(jst)
I like to have take() be used only in weird circumstances, so I think it is worth a little bit of change to make this non-weird looking.
Attachment #8486674 - Flags: review?(jst)
This enables some further changes.
Attachment #8486675 - Flags: review?(jst)
It looks like there was no refptr when this was written or something...
Attachment #8486677 - Flags: review?(jst)
This is odd.  I guess it was a different time. ;)
Attachment #8486680 - Flags: review?(jst)
This patch is slightly trickier than the rest, if you haven't fallen asleep already.

I see no reason why this is an array of pointers instead of just an array of structs.

Incidentally, this fixes a leak in nsWebBrowser::RemoveWebBrowserListener.  This code carefully deletes everything that gets removed when we empty the entire array, but when we delete one element, it doesn't.  I'm not sure if this code isn't run or what (it would show up on an LSan run but not a regular debug run, because the structs we're leaking don't have MOZ_COUNT_CTOR stuff.)
Attachment #8486683 - Flags: review?(jst)
Duplicate of this bug: 1064986
Summary: Fix up nsDocShellTreeOwner a little → Fix up nsDocShellTreeOwner and nsWebBrowser a little
This doesn't actually block anything I'm working on (I spent a bunch of time reading these files, but I haven't actually written any patches for them), so take your time.
Attachment #8486647 - Flags: review?(jst) → review+
Attachment #8486648 - Flags: review?(jst) → review+
Attachment #8486649 - Flags: review?(jst) → review+
Bug 1038756 is a huge patch that touches these files, so at bz's suggestion I'll wait until those have landed and then redo these.
Depends on: 1038756
Attachment #8486650 - Flags: review?(jst) → review+
Comment on attachment 8486652 [details] [diff] [review]
part 1e - Get rid of more space around parens in nsDocShellTreeOwner.

- In embedding/browser/nsDocShellTreeOwner.h:
-  NS_IMETHOD ShowTooltip ( int32_t inXCoords, int32_t inYCoords, const nsAString & inTipText ) ;
-  NS_IMETHOD HideTooltip ( ) ;
+  NS_IMETHOD ShowTooltip(int32_t inXCoords, int32_t inYCoords, const nsAString & inTipText );

Yank the space before ')' while you're at it :)

r=jst
Attachment #8486652 - Flags: review?(jst) → review+
Yeah, I need to do an extra pass here looking for " )".
and I guess " ;" if that's still a problem.
Attachment #8486653 - Flags: review?(jst) → review+
Attachment #8486654 - Flags: review?(jst) → review+
Attachment #8486660 - Flags: review?(jst) → review+
Attachment #8486662 - Flags: review?(jst) → review+
Attachment #8486663 - Flags: review?(jst) → review+
Attachment #8486664 - Flags: review?(jst) → review+
Attachment #8486671 - Flags: review?(jst) → review+
Attachment #8486673 - Flags: review?(jst) → review+
Attachment #8486674 - Flags: review?(jst) → review+
Attachment #8486675 - Flags: review?(jst) → review+
Attachment #8486677 - Flags: review?(jst) → review+
Attachment #8486678 - Flags: review?(jst) → review+
Comment on attachment 8486679 [details] [diff] [review]
part 6 - Use nsAutoPtr for nsWebBrowser::mInitInfo.

- In nsWebBrowser::InternalDestroy():

-   if (mInitInfo)
-      {
-      delete mInitInfo;
+   if (mInitInfo) {
       mInitInfo = nullptr;
-      }
+   }

No real need for the if check there, just set mInitInfo to nullptr unconditionally?
 
NS_IMETHODIMP nsWebBrowser::Destroy()
{
    InternalDestroy();
 
-   if (!mInitInfo)
+   if (!mInitInfo) {
       mInitInfo = new nsWebBrowserInitInfo();
+   }

This seems pretty wonky if you ask me... But that's probably for another day and another bug.
Attachment #8486679 - Flags: review?(jst) → review+
Attachment #8486680 - Flags: review?(jst) → review+
Comment on attachment 8486681 [details] [diff] [review]
part 8 - Convert nsWebBrowser::mListenerArray to an nsAutoPtr.

+   nsAutoPtr<nsTArray<nsWebBrowserListenerState*>> mListenerArray;

Doesn't msvc want a space between the two >'s here?
Attachment #8486681 - Flags: review?(jst) → review+
Attachment #8486683 - Flags: review?(jst) → review+
You'll probably want to wait for bug 1038756 to land before you land this change, likely much easier for you to deal with conflicts here than dealing with them in that bug. Thanks!
(In reply to Johnny Stenback  (:jst, jst@mozilla.com) from comment #30)
> You'll probably want to wait for bug 1038756 to land before you land this
> change, likely much easier for you to deal with conflicts here than dealing
> with them in that bug. Thanks!

Bug 1038756 landed a few weeks ago.  Thanks for letting us land first!
I finally got around to fixing this up.  I addressed the various review comments, except that >> is actually okay with MSV now, I believe, so I left that in.

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=28b6dc1a62c7
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.