Bug 263281 - Non-breaking API change required to support custom property editors in WPE
Summary: Non-breaking API change required to support custom property editors in WPE
Status: RESOLVED FIXED
Alias: None
Product: Java Server Faces
Classification: WebTools
Component: Core (show other bugs)
Version: 3.0.4   Edit
Hardware: PC Windows XP
: P1 blocker (vote)
Target Milestone: 3.0.4   Edit
Assignee: Cameron Bateman CLA
QA Contact:
URL:
Whiteboard: PMC_approved
Keywords:
Depends on:
Blocks:
 
Reported: 2009-02-02 14:08 EST by Cameron Bateman CLA
Modified: 2009-02-11 12:34 EST (History)
4 users (show)

See Also:
raghunathan.srinivasan: pmc_approved? (david_williams)
raghunathan.srinivasan: pmc_approved? (naci.dai)
deboer: pmc_approved+
neil.hauge: pmc_approved+
raghunathan.srinivasan: pmc_approved? (kaloyan)
raghunathan.srinivasan: review+

Attachments
Non-breaking API addition (13.30 KB, patch)
2009-02-02 14:08 EST, Cameron Bateman CLA 		no flags Details | Diff
Fixes for Gerry's code review (15.12 KB, patch)
2009-02-02 15:21 EST, Cameron Bateman CLA 		no flags Details | Diff
Added copyright headers (16.24 KB, patch)
2009-02-02 17:24 EST, Cameron Bateman CLA 		no flags Details | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron Bateman CLA 2009-02-02 14:08:01 EST 
Created attachment 124454 [details]
Non-breaking API addition

The attached patch adds a new extension point and a small, low-risk modification to internal code, which allows adopters to replace the property page that is provided for the Web Page Editor.
Comment 1 Cameron Bateman CLA 2009-02-02 15:21:21 EST 
Created attachment 124466 [details]
Fixes for Gerry's code review
Comment 2 Cameron Bateman CLA 2009-02-02 17:24:16 EST 
Created attachment 124483 [details]
Added copyright headers
Comment 3 Gerry Kessler CLA 2009-02-03 19:54:31 EST 
I've reviewed the patch, and it looks good.
Comment 4 Raghunathan Srinivasan CLA 2009-02-04 14:02:02 EST 
* Explain why you believe this is a stop-ship defect. Or, if it is a "hotbug" (requested by an adopter) please document it as such. 
This is a "hotbug" request from Oracle. The fix is required to provide enhanced property editor for the Web Page Editor feature in the adopter product.

* Is there a work-around? If so, why do you believe the work-around is insufficient? 
No workaround.

* How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added? 
Existing junits cover the change. The fix has also been verified in the adopter product.
* Give a brief technical overview. Who has reviewed this fix? 
See comment 0 . Gerry Kessler has reviewed the fix. See comment 3
* What is the risk associated with this fix? 
Low-risk.
Comment 5 Tim deBoer CLA 2009-02-04 14:21:36 EST 
+1

Out of curiosity, why have the IPropertySheetPageFactory at all? I thought a "lesson learned" from platform was to have the extension point use the abstract class directly with no interface. This is one less interface to maintain, and potential changes in the future are less likely to cause breakage if there is no chance of adopters incorrectly extending the interface.
Comment 6 Cameron Bateman CLA 2009-02-04 16:29:38 EST 
> I thought a "lesson learned" from platform was to have the extension point 
> use the abstract
> class directly with no interface. This is one less interface to maintain, and
> potential changes in the future are less likely to cause breakage if there is
> no chance of adopters incorrectly extending the interface.

My rationale for doing it this way is that we have more flexibility in the future if we need to implement an internal extension that is not dependent on constrained API code.

Could you point me to the platform doc on this?  What you suggest makes sense and I'm happy to get educated on best practice here.
Comment 7 Tim deBoer CLA 2009-02-04 17:17:26 EST 
Not quite sure what you mean about an internal extension since both IPropertySheetPageFactory and AbstractPropertySheetPageFactory are public (?).

Sorry, not aware of any docs on this. My general rule: interfaces for things you'll be hiding the implementing internally (like IProject), and abstract classes for things other teams will implement. Just learned from many hours with Jim des Rivieres (former Eclipse API doctor) a few years ago, and learning about cases like IPerspectiveListener where new extension interfaces kept having to be created to avoid breaking API. See IPerspectiveListener4 as an example. :)

Just to confirm, I've approved the bug either way.
Comment 8 Raghunathan Srinivasan CLA 2009-02-04 18:59:42 EST 
Marking it as PMC approved.
Comment 9 Cameron Bateman CLA 2009-02-04 19:57:39 EST 
Patch applied to 3.0.4.  Will mark fixed once 3.1 is synced.
Comment 10 Cameron Bateman CLA 2009-02-11 12:34:40 EST 
Change forward ported to 3.1 stream.