convert sites that QueryInterface to kBlockCID to use nsLayoutUtils::GetAsBlock

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: roc, Assigned: karunasagark)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: good first bug)

Attachments

(1 attachment, 2 obsolete attachments)

nsLayoutUtils::GetAsBlock provides a cleaner interface than using QueryInterface. 

Some sites that test frame->GetType() == nsGkAtoms::blockFrame/areaFrame should also use GetAsBlock instead and test for null.
Whiteboard: good first bug
Assignee

Comment 1

12 years ago
working on this ...
Assignee

Comment 2

12 years ago
Replacing the checks frame->GetType() == nsGkAtoms::blockFrame and frame->GetType() == nsGkAtoms::areaFrame seems unnecessary and would be asymmetrical. Any reason why we need to do this replacement??
I'm not sure what you mean by "asymmetrical". Doing this replacement where only blockFrame and areaFrame are being checked for (or when only blockFrame is being checked for but areaFrame *should* be checked for) should result in smaller simpler code in the caller.
+  block = nsLayoutUtils::GetAsBlock(aBlockFrame);
+  NS_ASSERTION(block,
                "Not a block frame?");

You can just eliminate "block" and write NS_ASSERTION(nsLayoutUtils::GetAsBlock(aBlockFrame), ...);

     nsBlockFrame* blockParent;
-    if (NS_SUCCEEDED(parent->QueryInterface(kBlockFrameCID, (void**)&blockParent)))
+    blockParent = nsLayoutUtils::GetAsBlock(parent);

Put the assignment in the same statement as the declaration,
     nsBlockFrame* blockParent = nsLayoutUtils::GetAsBlock(parent);

       void* bf;
-      if (NS_SUCCEEDED(f->QueryInterface(kBlockFrameCID, &bf))) {
+      bf = static_cast<void*>(nsLayoutUtils::GetAsBlock(f));
+      if (bf) {
         MarkAllDescendantLinesDirty(static_cast<nsBlockFrame*>(f));

As above, and also, make bf an nsBlockFrame* and remove these unnecessary casts. (Happens in lots of places)
Assignee

Comment 6

12 years ago
Attachment #294935 - Attachment is obsolete: true
Attachment #294947 - Flags: review?(roc)
Attachment #294935 - Flags: review?(roc)
 #ifdef DEBUG
-  nsBlockFrame* block;
-  NS_ASSERTION(NS_SUCCEEDED(aBlockFrame->QueryInterface(kBlockFrameCID,
-                                                        (void**)&block)) &&
-               block,
+  NS_ASSERTION(nsLayoutUtils::GetAsBlock(aBlockFrame),
                "Not a block frame?");
 #endif

You can remove #ifdef DEBUG because NS_ASSERTION does nothing in a non-DEBUG build.

 #ifdef DEBUG
   {
-    nsBlockFrame* block;
-    NS_ASSERTION(NS_SUCCEEDED(aBlockFrame->QueryInterface(kBlockFrameCID,
-                                                          (void**)&block)) &&
-                 block,
+    NS_ASSERTION(nsLayoutUtils::GetAsBlock(aBlockFrame),
                  "Not a block frame?");
   }
 #endif

Ditto

+      nsBlockFrame* bf =  nsLayoutUtils::GetAsBlock(f);

Remove extra space

+      if (bf) {
         MarkAllDescendantLinesDirty(static_cast<nsBlockFrame*>(f));

Use bf instead of the cast

     blockWithSpaceMgr = static_cast<nsBlockFrame*>(blockWithSpaceMgr->GetParent());

Ditto

   nsBlockFrame* block = static_cast<nsBlockFrame*>(aFrame);

Ditto

 #ifdef DEBUG
-  nsBlockFrame* blockFrame;
-  aBlock->QueryInterface(kBlockFrameCID, (void**)&blockFrame);
-  NS_ASSERTION(blockFrame, "aBlock must be a block");
+  NS_ASSERTION(nsLayoutUtils::GetAsBlock(aBlock), "aBlock must be a block");
 #endif

Remove unnecessary #ifdef DEBUG

 #ifdef DEBUG
-  nsBlockFrame* blockFrame;
-  aBlock->QueryInterface(kBlockFrameCID, (void**)&blockFrame);
-  NS_ASSERTION(blockFrame, "aBlock must be a block");
+  NS_ASSERTION(nsLayoutUtils::GetAsBlock(aBlock), "aBlock must be a block");
 #endif

Ditto

+      reachedBlockAncestor = (nsLayoutUtils::GetAsBlock(frame)) ? PR_TRUE : PR_FALSE;

Make this nsLayoutUtils::GetAsBlock(frame) != nsnull

+      reachedBlockAncestor = (nsLayoutUtils::GetAsBlock(frame)) ? PR_TRUE : PR_FALSE;

Ditto
Assignee

Comment 8

12 years ago
Attachment #294947 - Attachment is obsolete: true
Attachment #295022 - Flags: review?(roc)
Attachment #294947 - Flags: review?(roc)
Comment on attachment 295022 [details] [diff] [review]
convert sites that QueryInterface to kBlockCID to use nsLayoutUtils::GetAsBlock

cool.

We probably don't need to take this for FF3 though. Let's land it post-FF3.
Attachment #295022 - Flags: superreview+
Attachment #295022 - Flags: review?(roc)
Attachment #295022 - Flags: review+
Assignee: nobody → karunasagark
OS: Mac OS X → All
Hardware: PC → All
Status: NEW → ASSIGNED
Hmm, I need to land this.
Pushed 140ebc8ba146.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.